Avoid calling deprecated arena-enabled constructors in arena.h.

ClaytonKnittel · mkruskal-google · commit 813a7ef2efbe · 2025-10-09T09:50:46.000-07:00
This caused log spam in OSS protobuf. We can call the `InternalVisibility` constructors instead, which behave the same but aren't deprecated.

PiperOrigin-RevId: 816437432
diff --git a/src/google/protobuf/arena.h b/src/google/protobuf/arena.h
@@ -124,6 +124,16 @@ constexpr bool FieldHasArenaOffset() {
   return !std::is_same_v<T, ArenaRepT>;
 }
 
+// TODO - Some types have a deprecated arena-enabled constructor,
+// as we plan to remove it in favor of using arena offsets, but for now Arena
+// needs to call it. While the arena constructor exists, we will call the
+// `InternalVisibility` override to silence the warning.
+template <typename T>
+constexpr bool HasDeprecatedArenaConstructor() {
+  return std::is_base_of_v<internal::RepeatedPtrFieldBase, T> &&
+         !std::is_same_v<T, internal::RepeatedPtrFieldBase>;
+}
+
 template <typename T>
 void arena_delete_object(void* PROTOBUF_NONNULL object) {
   delete reinterpret_cast<T*>(object);
@@ -478,16 +488,12 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8)
     static T* PROTOBUF_NONNULL ConstructOnArena(void* PROTOBUF_NONNULL ptr,
                                                 Arena& arena, Args&&... args) {
       // TODO - ClangTidy gives warnings for calling the deprecated
-      // `RepeatedPtrField(Arena*)` constructor here, but this is the correct
-      // way to call it as it will allow us to silently switch to a different
-      // constructor once arena pointers are removed from RepeatedPtrFields.
-      // While this constructor exists, we will call the `InternalVisibility`
-      // override to silence the warning.
-      //
-      // Note: RepeatedPtrFieldBase is sometimes constructed internally, and it
-      // doesn't have `InternalVisibility` constructors.
-      if constexpr (std::is_base_of_v<internal::RepeatedPtrFieldBase, T> &&
-                    !std::is_same_v<T, internal::RepeatedPtrFieldBase>) {
+      // constructors here, which leads to log spam. It is correct to invoke
+      // these constructors through the Arena class as it will allow us to
+      // silently switch to a different constructor once arena pointers are
+      // removed. While these constructors exists, we will call the
+      // `InternalVisibility` overrides to silence the warning.
+      if constexpr (internal::HasDeprecatedArenaConstructor<T>()) {
         return new (ptr) T(internal::InternalVisibility(), &arena,
                            static_cast<Args&&>(args)...);
       } else {
@@ -510,7 +516,8 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8)
       // Fields which use arena offsets don't have constructors that take an
       // arena pointer. Since the arena is nullptr, it is safe to default
       // construct the object.
-      if constexpr (internal::FieldHasArenaOffset<T>()) {
+      if constexpr (internal::FieldHasArenaOffset<T>() ||
+                    internal::HasDeprecatedArenaConstructor<T>()) {
         return new T();
       } else {
         return new T(nullptr);
@@ -590,7 +597,8 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8)
     static_assert(is_arena_constructable<T>::value,
                   "Can only construct types that are ArenaConstructable");
     if (ABSL_PREDICT_FALSE(arena == nullptr)) {
-      if constexpr (internal::FieldHasArenaOffset<T>()) {
+      if constexpr (internal::FieldHasArenaOffset<T>() ||
+                    internal::HasDeprecatedArenaConstructor<T>()) {
         return new T(static_cast<Args&&>(args)...);
       } else {
         return new T(nullptr, static_cast<Args&&>(args)...);
@@ -744,7 +752,12 @@ Arena::DefaultConstruct(Arena* PROTOBUF_NULLABLE arena) {
   static_assert(is_destructor_skippable<T>::value, "");
   void* mem = arena != nullptr ? arena->AllocateAligned(sizeof(T))
                                : ::operator new(sizeof(T));
-  return new (mem) T(arena);
+
+  if constexpr (internal::HasDeprecatedArenaConstructor<T>()) {
+    return new (mem) T(internal::InternalVisibility(), arena);
+  } else {
+    return new (mem) T(arena);
+  }
 }
 
 template <typename T>
diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h
@@ -1022,7 +1022,7 @@ class ABSL_ATTRIBUTE_WARN_UNUSED RepeatedPtrField final
 
   // Arena enabled constructors: for internal use only.
   RepeatedPtrField(internal::InternalVisibility, Arena* arena)
-      : RepeatedPtrField(arena) {}
+      : RepeatedPtrFieldBase(arena) {}
   RepeatedPtrField(internal::InternalVisibility, Arena* arena,
                    const RepeatedPtrField& rhs)
       : RepeatedPtrField(arena, rhs) {}
