Addressed PR comments.

upb/mini_table.c

@@ -111,10 +111,14 @@ char upb_FromBase92(uint8_t ch) {
 }
 
 bool upb_IsTypePackable(upb_FieldType type) {
-  static const unsigned kPackableTypes =
-      -1U & ~(1 << kUpb_FieldType_String) & ~(1U << kUpb_FieldType_Bytes) &
-      ~(1U << kUpb_FieldType_Message) & ~(1U << kUpb_FieldType_Group);
-  return (1 << type) & kPackableTypes;
+  // clang-format off
+  static const unsigned kUnpackableTypes =
+      (1 << kUpb_FieldType_String) |
+      (1 << kUpb_FieldType_Bytes) |
+      (1 << kUpb_FieldType_Message) |
+      (1 << kUpb_FieldType_Group);
+  // clang-format on
+  return (1 << type) & ~kUnpackableTypes;
 }
 
 /** upb_MtDataEncoder *********************************************************/
@@ -178,7 +182,6 @@ char* upb_MtDataEncoder_StartMessage(upb_MtDataEncoder* e, char* ptr,
   return upb_MtDataEncoder_PutModifier(e, ptr, msg_mod);
 }
 
-#include <stdio.h>
 char* upb_MtDataEncoder_PutField(upb_MtDataEncoder* e, char* ptr,
                                  upb_FieldType type, uint32_t field_num,
                                  uint64_t field_mod) {
@@ -229,10 +232,12 @@ char* upb_MtDataEncoder_PutField(upb_MtDataEncoder* e, char* ptr,
     // are bit flags).
     encoded_type += kUpb_EncodedType_RepeatedBase;
 
-    bool field_is_packed = field_mod & kUpb_FieldModifier_IsPacked;
-    bool default_is_packed = in->msg_mod & kUpb_MessageModifier_DefaultIsPacked;
-    if (field_is_packed != default_is_packed && upb_IsTypePackable(type)) {
-      encoded_modifiers |= kUpb_EncodedFieldModifier_FlipPacked;
+    if (upb_IsTypePackable(type)) {
+      bool field_is_packed = field_mod & kUpb_FieldModifier_IsPacked;
+      bool default_is_packed = in->msg_mod & kUpb_MessageModifier_DefaultIsPacked;
+      if (field_is_packed != default_is_packed) {
+        encoded_modifiers |= kUpb_EncodedFieldModifier_FlipPacked;
+      }
     }
   }
   ptr = upb_MtDataEncoder_Put(e, ptr, encoded_type);
@@ -374,18 +379,19 @@ static const char* upb_MiniTable_DecodeBase92Varint(upb_MtDecoder* d,
 
 static bool upb_MiniTable_HasSub(upb_MiniTable_Field* field,
                                  uint64_t msg_modifiers) {
-  if (field->descriptortype == kUpb_FieldType_Message ||
-      field->descriptortype == kUpb_FieldType_Group) {
-    return true;
-  } else if (field->descriptortype == kUpb_FieldType_Enum) {
-    return true;
-  } else if (field->descriptortype == kUpb_FieldType_String) {
-    if (!(msg_modifiers & kUpb_MessageModifier_ValidateUtf8)) {
-      field->descriptortype = kUpb_FieldType_Bytes;
+  switch (field->descriptortype) {
+    case kUpb_FieldType_Message:
+    case kUpb_FieldType_Group:
+    case kUpb_FieldType_Enum:
+      return true;
+    case kUpb_FieldType_String:
+      if (!(msg_modifiers & kUpb_MessageModifier_ValidateUtf8)) {
+        field->descriptortype = kUpb_FieldType_Bytes;
+      }
+      return false;
+    default:
       return false;
-    }
   }
-  return false;
 }
 
 static bool upb_MtDecoder_FieldIsPackable(upb_MiniTable_Field* field) {
@@ -474,7 +480,6 @@ static void upb_MiniTable_SetField(upb_MtDecoder* d, uint8_t ch,
                               msg_modifiers);
 }
 
-#include <stdio.h>
 static void upb_MtDecoder_ModifyField(upb_MtDecoder* d,
                                       uint32_t message_modifiers,
                                       uint32_t field_modifiers,
@@ -996,7 +1001,7 @@ upb_MiniTable_Enum* upb_MiniTable_BuildEnum(const char* data, size_t len,
 
   if (ptr != end) {
     size_t bytes = end - ptr;
-    if (bytes / 4 * 4 != bytes) {
+    if (bytes % 4 != 0) {
       upb_MtDecoder_ErrorFormat(&decoder, "Bytes should be a multiple of 4");
       UPB_UNREACHABLE();
     }

upb/msg_internal.h

@@ -84,12 +84,10 @@ typedef enum {
   kUpb_FieldMode_Map = 0,
   kUpb_FieldMode_Array = 1,
   kUpb_FieldMode_Scalar = 2,
-
 } upb_FieldMode;
 
-#define kUpb_FieldMode_Mask                        \
-  3 /* Mask to isolate the mode from upb_FieldRep. \
-     */
+// Mask to isolate the upb_FieldMode from field.mode.
+#define kUpb_FieldMode_Mask 3
 
 /* Extra flags on the mode field. */
 typedef enum {

upbc/protoc-gen-upb.cc

@@ -389,11 +389,10 @@ class FilePlatformLayout {
   const char* AllocStr(const std::string& str);
 
  private:
-  typedef absl::flat_hash_map<const protobuf::Descriptor*, upb_MiniTable*>
-      TableMap;
-  typedef absl::flat_hash_map<const protobuf::FieldDescriptor*,
-                              upb_MiniTable_Extension>
-      ExtensionMap;
+  using TableMap =
+      absl::flat_hash_map<const protobuf::Descriptor*, upb_MiniTable*>;
+  using ExtensionMap = absl::flat_hash_map<const protobuf::FieldDescriptor*,
+                                           upb_MiniTable_Extension>;
   upb::Arena arena_;
   TableMap table_map_;
   ExtensionMap extension_map_;
@@ -420,8 +419,10 @@ void FilePlatformLayout::ResolveIntraFileReferences() {
   for (const auto& pair : table_map_) {
     upb_MiniTable* mt = pair.second;
     // First we properly resolve for defs within the file.
-    for (const auto& f : FieldNumberOrder(pair.first)) {
+    for (const auto* f : FieldNumberOrder(pair.first)) {
       if (f->message_type() && f->message_type()->file() == f->file()) {
+        // const_cast is safe because the mini-table is owned exclusively
+        // by us, and was allocated from an arena (known-writable memory).
         upb_MiniTable_Field* mt_f = const_cast<upb_MiniTable_Field*>(
             upb_MiniTable_FindFieldByNumber(mt, f->number()));
         upb_MiniTable* sub_mt = GetMiniTable(f->message_type());
@@ -456,18 +457,20 @@ std::string FilePlatformLayout::GetSub(upb_MiniTable_Sub sub) {
     default:
       return std::string("{.submsg = NULL}");
   }
-  return std::string("ERROR in WriteSub");
+  return std::string("ERROR in GetSub");
 }
 
 void FilePlatformLayout::SetSubTableStrings() {
   for (const auto& pair : table_map_) {
     upb_MiniTable* mt = pair.second;
-    for (const auto& f : FieldNumberOrder(pair.first)) {
+    for (const auto* f : FieldNumberOrder(pair.first)) {
       upb_MiniTable_Field* mt_f = const_cast<upb_MiniTable_Field*>(
           upb_MiniTable_FindFieldByNumber(mt, f->number()));
       assert(mt_f);
       upb_MiniTable_Sub sub = PackSubForField(f, mt_f);
       if (IsNull(sub)) continue;
+      // const_cast is safe because the mini-table is owned exclusively
+      // by us, and was allocated from an arena (known-writable memory).
       *const_cast<upb_MiniTable_Sub*>(&mt->subs[mt_f->submsg_index]) = sub;
     }
   }
@@ -502,9 +505,9 @@ void FilePlatformLayout::BuildMiniTables(const protobuf::FileDescriptor* fd) {
 }
 
 void FilePlatformLayout::BuildExtensions(const protobuf::FileDescriptor* fd) {
-  auto sorted = SortedExtensions(fd);
+  std::vector<const protobuf::FieldDescriptor*> sorted = SortedExtensions(fd);
   upb::Status status;
-  for (const auto& f : sorted) {
+  for (const auto* f : sorted) {
     upb::MtDataEncoder e;
     e.StartMessage(0);
     e.PutField(static_cast<upb_FieldType>(f->type()), f->number(),
@@ -517,10 +520,10 @@ void FilePlatformLayout::BuildExtensions(const protobuf::FileDescriptor* fd) {
       fprintf(stderr, "Error building mini-table: %s\n",
               status.error_message());
     }
+    ABSL_ASSERT(ok);
     ext.extendee = reinterpret_cast<const upb_MiniTable*>(
         AllocStr(MessageInit(f->containing_type())));
     ext.sub = PackSubForField(f, &ext.field);
-    ABSL_ASSERT(ok);
   }
 }
 
@@ -545,7 +548,7 @@ upb_MiniTable* FilePlatformLayout::MakeRegularMiniTable(
     const protobuf::Descriptor* m) {
   upb::MtDataEncoder e;
   e.StartMessage(GetMessageModifiers(m));
-  for (const auto& f : FieldNumberOrder(m)) {
+  for (const auto* f : FieldNumberOrder(m)) {
     e.PutField(static_cast<upb_FieldType>(f->type()), f->number(),
                GetFieldModifiers(f));
   }
@@ -557,7 +560,7 @@ upb_MiniTable* FilePlatformLayout::MakeRegularMiniTable(
       e.PutOneofField(f->number());
     }
   }
-  const auto& str = e.data();
+  const std::string& str = e.data();
   upb::Status status;
   upb_MiniTable* ret = upb_MiniTable_Build(str.data(), str.size(), platform_,
                                            arena_.ptr(), status.ptr());