Streamline creation of new elements in RepeatedPtrField

The logic to create new elements in `RepeatedPtrField` was spread across an assorted collection of specializations of `GenericTypeHandler`, individual `GenericTypeHandler` static functions, and external member and non-member functions, all living in multiple sources. Now all that logic is concentrated in three symmetric `GenericTypeHandler` specializations. PiperOrigin-RevId: 674064544
deannagarcia · Sep 13, 2024 · 91e3162 · 91e3162
1 parent 5254448
commit 91e3162
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 128 deletions.
diff --git a/src/google/protobuf/extension_set.cc b/src/google/protobuf/extension_set.cc
@@ -825,7 +825,7 @@ MessageLite* ExtensionSet::AddMessage(int number, FieldType type,
 
   return reinterpret_cast<internal::RepeatedPtrFieldBase*>(
              extension->ptr.repeated_message_value)
-      ->AddMessage(&prototype);
+      ->AddFromPrototype(&prototype);
 }
 
 // Defined in extension_set_heavy.cc.

diff --git a/src/google/protobuf/lazy_repeated_field.h b/src/google/protobuf/lazy_repeated_field.h
@@ -328,7 +328,7 @@ class LazyRepeatedPtrField {
     const char* ptr2 = ptr;
     TagType next_tag;
     do {
-      MessageLite* submsg = value->AddMessage(prototype);
+      MessageLite* submsg = value->AddFromPrototype(prototype);
       // ptr2 points to the start of the element's encoded length.
       ptr = ctx->ParseMessage(submsg, ptr2);
       if (PROTOBUF_PREDICT_FALSE(ptr == nullptr)) return nullptr;

diff --git a/src/google/protobuf/message.cc b/src/google/protobuf/message.cc
@@ -495,28 +495,6 @@ const internal::RepeatedFieldAccessor* Reflection::RepeatedFieldAccessor(
 }
 
 namespace internal {
-template <>
-#if defined(_MSC_VER) && (_MSC_VER >= 1800)
-// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue
-// #240
-PROTOBUF_NOINLINE
-#endif
-    Message*
-    GenericTypeHandler<Message>::NewFromPrototype(const Message* prototype,
-                                                  Arena* arena) {
-  return prototype->New(arena);
-}
-template <>
-#if defined(_MSC_VER) && (_MSC_VER >= 1800)
-// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue
-// #240
-PROTOBUF_NOINLINE
-#endif
-    Arena*
-    GenericTypeHandler<Message>::GetArena(Message* value) {
-  return value->GetArena();
-}
-
 template void InternalMetadata::DoClear<UnknownFieldSet>();
 template void InternalMetadata::DoMergeFrom<UnknownFieldSet>(
     const UnknownFieldSet& other);

diff --git a/src/google/protobuf/message_lite.cc b/src/google/protobuf/message_lite.cc
@@ -711,16 +711,6 @@ absl::Cord MessageLite::SerializePartialAsCord() const {
 
 namespace internal {
 
-MessageLite* NewFromPrototypeHelper(const MessageLite* prototype,
-                                    Arena* arena) {
-  return prototype->New(arena);
-}
-template <>
-void GenericTypeHandler<MessageLite>::Merge(const MessageLite& from,
-                                            MessageLite* to) {
-  to->CheckTypeAndMergeFrom(from);
-}
-
 // Non-inline variants of std::string specializations for
 // various InternalMetadata routines.
 template <>

diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc
@@ -93,14 +93,6 @@ void RepeatedPtrFieldBase::DestroyProtos() {
   tagged_rep_or_elem_ = nullptr;
 }
 
-void* RepeatedPtrFieldBase::AddMessageLite(ElementFactory factory) {
-  return AddInternal(factory);
-}
-
-void* RepeatedPtrFieldBase::AddString() {
-  return AddInternal([](Arena* arena) { return NewStringElement(arena); });
-}
-
 void RepeatedPtrFieldBase::CloseGap(int start, int num) {
   if (using_sso()) {
     if (start == 0 && num == 1) {
@@ -116,11 +108,6 @@ void RepeatedPtrFieldBase::CloseGap(int start, int num) {
   ExchangeCurrentSize(current_size_ - num);
 }
 
-MessageLite* RepeatedPtrFieldBase::AddMessage(const MessageLite* prototype) {
-  return static_cast<MessageLite*>(
-      AddInternal([prototype](Arena* a) { return prototype->New(a); }));
-}
-
 void InternalOutOfLineDeleteMessageLite(MessageLite* message) {
   delete message;
 }
@@ -227,10 +214,6 @@ void RepeatedPtrFieldBase::MergeFrom<MessageLite>(
   }
 }
 
-void* NewStringElement(Arena* arena) {
-  return Arena::Create<std::string>(arena);
-}
-
 }  // namespace internal
 }  // namespace protobuf
 }  // namespace google

diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h
@@ -108,7 +108,8 @@ class GenericTypeHandler;
 //     using Type = MyType;
 //     using Movable = ...;
 //
-//     static Type*(*)(Arena*) GetNewFunc();
+//     static ElementFactory GetNewFunc();
+//     static ElementFactory GetNewFromPrototypeFunc(const Type* prototype);
 //     static void GetArena(Type* value);
 //
 //     static Type* New(Arena* arena);
@@ -127,8 +128,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
 
   static constexpr int kSSOCapacity = 1;
 
-  using ElementFactory = void* (*)(Arena*);
-
  protected:
   // We use the same TypeHandler for all Message types to deduplicate generated
   // code.
@@ -192,15 +191,18 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
 
   template <typename TypeHandler>
   Value<TypeHandler>* Add() {
-    if (std::is_same<Value<TypeHandler>, std::string>{}) {
-      return cast<TypeHandler>(AddString());
-    }
-    return cast<TypeHandler>(AddMessageLite(TypeHandler::GetNewFunc()));
+    return cast<TypeHandler>(AddInternal(TypeHandler::GetNewFunc()));
   }
 
-  template <
-      typename TypeHandler,
-      typename std::enable_if<TypeHandler::Movable::value>::type* = nullptr>
+  template <typename Type>
+  Type* AddFromPrototype(const Type* prototype) {
+    using TypeHandler = GenericTypeHandler<Type>;
+    using H = CommonHandler<TypeHandler>;
+    return cast<TypeHandler>(
+        AddInternal(H::GetNewFromPrototypeFunc(prototype)));
+  }
+
+  template <typename TypeHandler>
   inline void Add(Value<TypeHandler>&& value) {
     if (current_size_ < allocated_size()) {
       *cast<TypeHandler>(element_at(ExchangeCurrentSize(current_size_ + 1))) =
@@ -255,12 +257,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
     return *cast<TypeHandler>(element_at(index));
   }
 
-  // Creates and adds an element using the given prototype, without introducing
-  // a link-time dependency on the concrete message type.
-  //
-  // Pre-condition: prototype must not be nullptr.
-  MessageLite* AddMessage(const MessageLite* prototype);
-
   template <typename TypeHandler>
   void Clear() {
     const int n = current_size_;
@@ -725,10 +721,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
     return InternalExtend(n - Capacity());
   }
 
-  // Internal helpers for Add that keep definition out-of-line.
-  void* AddMessageLite(ElementFactory factory);
-  void* AddString();
-
   // Common implementation used by various Add* methods. `factory` is an object
   // used to construct a new element unless there are spare cleared elements
   // ready for reuse. Returns pointer to the new element.
@@ -813,13 +805,32 @@ void* RepeatedPtrFieldBase::AddInternal(Factory factory) {
 
 PROTOBUF_EXPORT void InternalOutOfLineDeleteMessageLite(MessageLite* message);
 
+// Encapsulates the minimally required subset of T's properties in a
+// `RepeatedPtrField<T>` specialization so the type-agnostic
+// `RepeatedPtrFieldBase` could do its job without knowing T.
+//
+// This generic definition is for types derived from `MessageLite`. That is
+// statically asserted, but only where a non-conforming type would emit a
+// compile-time diagnostic that lacks proper guidance for fixing. Asserting
+// at the top level isn't possible, because some template argument types are not
+// yet fully defined at the instantiation point.
+//
+// Explicit specializations are provided for `std::string` and
+// `StringPieceField` further below.
 template <typename GenericType>
 class GenericTypeHandler {
  public:
   using Type = GenericType;
   using Movable = IsMovable<Type>;
 
-  static constexpr auto GetNewFunc() { return Arena::DefaultConstruct<Type>; }
+  static constexpr auto* GetNewFunc() { return Arena::DefaultConstruct<Type>; }
+  static constexpr auto GetNewFromPrototypeFunc(const Type* prototype) {
+    static_assert(std::is_base_of<MessageLite, Type>::value, "");
+    return [prototype](Arena* arena) {
+      ABSL_DCHECK(prototype != nullptr);
+      return prototype->New(arena);
+    };
+  }
   static inline Arena* GetArena(Type* value) {
     return Arena::InternalGetArena(value);
   }
@@ -830,76 +841,42 @@ class GenericTypeHandler {
   static inline Type* New(Arena* arena, Type&& value) {
     return Arena::Create<Type>(arena, std::move(value));
   }
-  static inline Type* NewFromPrototype(const Type* /*prototype*/,
+  static inline Type* NewFromPrototype(const Type* prototype,
                                        Arena* arena = nullptr) {
-    return New(arena);
+    static_assert(std::is_base_of<MessageLite, Type>::value, "");
+    ABSL_DCHECK(prototype != nullptr);
+    return prototype->New(arena);
   }
   static inline void Delete(Type* value, Arena* arena) {
     if (arena != nullptr) return;
-#ifdef __cpp_if_constexpr
-    if constexpr (std::is_base_of<MessageLite, Type>::value) {
-      // Using virtual destructor to reduce generated code size that would have
-      // happened otherwise due to inlined `~Type()`.
-      InternalOutOfLineDeleteMessageLite(value);
-    } else {
-      delete value;
-    }
-#else
-    delete value;
-#endif
+    // Using virtual destructor to reduce generated code size that would have
+    // happened otherwise due to inlined `~Type()`.
+    InternalOutOfLineDeleteMessageLite(value);
+  }
+  static inline void Clear(Type* value) {
+    static_assert(std::is_base_of<MessageLite, Type>::value, "");
+    value->Clear();
+  }
+  static inline void Merge(const Type& from, Type* to) {
+    static_assert(std::is_base_of<MessageLite, Type>::value, "");
+    to->CheckTypeAndMergeFrom(from);
   }
-  static inline void Clear(Type* value) { value->Clear(); }
-  static void Merge(const Type& from, Type* to);
   static inline size_t SpaceUsedLong(const Type& value) {
+    static_assert(std::is_base_of<MessageLite, Type>::value, "");
     return value.SpaceUsedLong();
   }
 };
 
-// NewFromPrototypeHelper() is not defined inline here, as we will need to do a
-// virtual function dispatch anyways to go from Message* to call New/Merge. (The
-// additional helper is needed as a workaround for MSVC.)
-PROTOBUF_EXPORT MessageLite* NewFromPrototypeHelper(
-    const MessageLite* prototype, Arena* arena);
-
-template <>
-inline MessageLite* GenericTypeHandler<MessageLite>::NewFromPrototype(
-    const MessageLite* prototype, Arena* arena) {
-  return NewFromPrototypeHelper(prototype, arena);
-}
-template <>
-inline Arena* GenericTypeHandler<MessageLite>::GetArena(MessageLite* value) {
-  return value->GetArena();
-}
-
-template <typename GenericType>
-PROTOBUF_NOINLINE inline void GenericTypeHandler<GenericType>::Merge(
-    const GenericType& from, GenericType* to) {
-  to->MergeFrom(from);
-}
-template <>
-PROTOBUF_EXPORT void GenericTypeHandler<MessageLite>::Merge(
-    const MessageLite& from, MessageLite* to);
-
-// Message specialization bodies defined in message.cc. This split is necessary
-// to allow proto2-lite (which includes this header) to be independent of
-// Message.
-template <>
-PROTOBUF_EXPORT Message* GenericTypeHandler<Message>::NewFromPrototype(
-    const Message* prototype, Arena* arena);
-template <>
-PROTOBUF_EXPORT Arena* GenericTypeHandler<Message>::GetArena(Message* value);
-
-PROTOBUF_EXPORT void* NewStringElement(Arena* arena);
-
 template <>
 class GenericTypeHandler<std::string> {
  public:
   using Type = std::string;
   using Movable = IsMovable<Type>;
 
-  static constexpr auto GetNewFunc() { return NewStringElement; }
-  static inline Arena* GetArena(Type*) { return nullptr; }
-
+  static constexpr auto* GetNewFunc() { return Arena::Create<Type>; }
+  static constexpr auto* GetNewFromPrototypeFunc(const Type* prototype) {
+    return GetNewFunc();
+  }
   static PROTOBUF_NOINLINE Type* New(Arena* arena) {
     return Arena::Create<Type>(arena);
   }
@@ -914,13 +891,15 @@ class GenericTypeHandler<std::string> {
       delete value;
     }
   }
+  static inline Arena* GetArena(Type*) { return nullptr; }
   static inline void Clear(Type* value) { value->clear(); }
   static inline void Merge(const Type& from, Type* to) { *to = from; }
-  static size_t SpaceUsedLong(const Type& value) {
+  static inline size_t SpaceUsedLong(const Type& value) {
     return sizeof(value) + StringSpaceUsedExcludingSelfLong(value);
   }
 };
 
+
 }  // namespace internal
 
 // RepeatedPtrField is like RepeatedField, but used for repeated strings or
@@ -1246,7 +1225,7 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
 
 
   void AddAllocatedForParse(Element* p) {
-    return RepeatedPtrFieldBase::AddAllocatedForParse(p);
+    RepeatedPtrFieldBase::AddAllocatedForParse(p);
   }
 };