hpb: composition over inheritance - ExtensionIdentifier

Have ExtensionIdentifier have a upb_MiniTableExtension* instead of subclassing a class that is just a plain holder for a upb_MiniTableExtension* with a getter with the same name We generally want to lean toward composition over inheritance. Without a compelling reason for inheritance, let's prefer composition for clarity, flexibility, and evolvability. Locking oneself into an inheritance hierarchy has marred many a codebase. PiperOrigin-RevId: 689380813
protocolbuffers · Oct 24, 2024 · 350e24e · 350e24e
1 parent 7f6e90b
commit 350e24e
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 29 deletions.
diff --git a/hpb/extension.h b/hpb/extension.h
@@ -34,19 +34,6 @@ absl::Status SetExtension(upb_Message* message, upb_Arena* message_arena,
                           const upb_MiniTableExtension* ext,
                           const upb_Message* extension);
 
-class ExtensionMiniTableProvider {
- public:
-  constexpr explicit ExtensionMiniTableProvider(
-      const upb_MiniTableExtension* mini_table_ext)
-      : mini_table_ext_(mini_table_ext) {}
-  const upb_MiniTableExtension* mini_table_ext() const {
-    return mini_table_ext_;
-  }
-
- private:
-  const upb_MiniTableExtension* mini_table_ext_;
-};
-
 // -------------------------------------------------------------------
 // ExtensionIdentifier
 // This is the type of actual extension objects.  E.g. if you have:
@@ -56,20 +43,25 @@ class ExtensionMiniTableProvider {
 // then "bar" will be defined in C++ as:
 //   ExtensionIdentifier<Foo, MyExtension> bar(&namespace_bar_ext);
 template <typename ExtendeeType, typename ExtensionType>
-class ExtensionIdentifier : public ExtensionMiniTableProvider {
+class ExtensionIdentifier {
  public:
   using Extension = ExtensionType;
   using Extendee = ExtendeeType;
 
+  // Placeholder for extant legacy callers, avoid use if possible
+  const upb_MiniTableExtension* mini_table_ext() const {
+    return mini_table_ext_;
+  }
+
  private:
-  constexpr explicit ExtensionIdentifier(
-      const upb_MiniTableExtension* mini_table_ext)
-      : ExtensionMiniTableProvider(mini_table_ext) {}
+  constexpr explicit ExtensionIdentifier(const upb_MiniTableExtension* mte)
+      : mini_table_ext_(mte) {}
   constexpr uint32_t number() const {
-    return upb_MiniTableExtension_Number(mini_table_ext());
+    return upb_MiniTableExtension_Number(mini_table_ext_);
   }
 
   friend struct PrivateAccess;
+  const upb_MiniTableExtension* mini_table_ext_;
 };
 
 upb_ExtensionRegistry* GetUpbExtensions(
@@ -80,13 +72,12 @@ upb_ExtensionRegistry* GetUpbExtensions(
 class ExtensionRegistry {
  public:
   ExtensionRegistry(
-      const std::vector<const internal::ExtensionMiniTableProvider*>&
-          extensions,
+      const std::vector<const upb_MiniTableExtension*>& extensions,
       const upb::Arena& arena)
       : registry_(upb_ExtensionRegistry_New(arena.ptr())) {
     if (registry_) {
-      for (const auto& ext_provider : extensions) {
-        const auto* ext = ext_provider->mini_table_ext();
+      for (const auto extension : extensions) {
+        const auto* ext = extension;
         bool success = upb_ExtensionRegistry_AddArray(registry_, &ext, 1);
         if (!success) {
           registry_ = nullptr;

diff --git a/hpb/internal/message_lock_test.cc b/hpb/internal/message_lock_test.cc
@@ -11,6 +11,7 @@
 #include <mutex>
 #include <string>
 #include <thread>
+#include <vector>
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -19,6 +20,7 @@
 #include "google/protobuf/compiler/hpb/tests/test_model.upb.proto.h"
 #include "google/protobuf/hpb/hpb.h"
 #include "upb/mem/arena.hpp"
+#include "upb/mini_table/extension.h"
 
 #ifndef ASSERT_OK
 #define ASSERT_OK(x) ASSERT_TRUE(x.ok())
@@ -109,14 +111,18 @@ TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceBothLazy) {
 
 TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceOneLazyOneEager) {
   ::upb::Arena arena;
-  TestConcurrentExtensionAccess({{&theme}, arena});
-  TestConcurrentExtensionAccess({{&ThemeExtension::theme_extension}, arena});
+  std::vector<const upb_MiniTableExtension*> e1{theme.mini_table_ext()};
+  TestConcurrentExtensionAccess({e1, arena});
+  std::vector<const upb_MiniTableExtension*> e2{
+      ThemeExtension::theme_extension.mini_table_ext()};
+  TestConcurrentExtensionAccess({e2, arena});
 }
 
 TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceBothEager) {
   ::upb::Arena arena;
-  TestConcurrentExtensionAccess(
-      {{&theme, &ThemeExtension::theme_extension}, arena});
+  std::vector<const upb_MiniTableExtension*> exts{
+      theme.mini_table_ext(), ThemeExtension::theme_extension.mini_table_ext()};
+  TestConcurrentExtensionAccess({exts, arena});
 }
 
 }  // namespace

diff --git a/hpb_generator/tests/test_generated.cc b/hpb_generator/tests/test_generated.cc
@@ -25,12 +25,14 @@
 #include "google/protobuf/compiler/hpb/tests/test_model.upb.proto.h"
 #include "google/protobuf/hpb/arena.h"
 #include "google/protobuf/hpb/backend/upb/interop.h"
+#include "google/protobuf/hpb/extension.h"
 #include "google/protobuf/hpb/hpb.h"
 #include "google/protobuf/hpb/ptr.h"
 #include "google/protobuf/hpb/repeated_field.h"
 #include "google/protobuf/hpb/requires.h"
 #include "upb/mem/arena.h"
 #include "upb/mem/arena.hpp"
+#include "upb/mini_table/extension.h"
 
 namespace {
 
@@ -982,8 +984,11 @@ TEST(CppGeneratedCode, ParseWithExtensionRegistry) {
   ::upb::Arena arena;
   auto bytes = ::hpb::Serialize(&model, arena);
   EXPECT_EQ(true, bytes.ok());
-  ::hpb::ExtensionRegistry extensions(
-      {&theme, &other_ext, &ThemeExtension::theme_extension}, arena);
+  std::vector<const upb_MiniTableExtension*> exts{
+      theme.mini_table_ext(), other_ext.mini_table_ext(),
+      ThemeExtension::theme_extension.mini_table_ext()};
+
+  ::hpb::ExtensionRegistry extensions(exts, arena);
   TestModel parsed_model =
       ::hpb::Parse<TestModel>(bytes.value(), extensions).value();
   EXPECT_EQ("Test123", parsed_model.str1());
@@ -1247,7 +1252,8 @@ TEST(CppGeneratedCode, HasExtensionAndRegistry) {
   std::string data = std::string(::hpb::Serialize(&source, arena).value());
 
   // Test with ExtensionRegistry
-  ::hpb::ExtensionRegistry extensions({&theme}, arena);
+  std::vector<const upb_MiniTableExtension*> exts{theme.mini_table_ext()};
+  hpb::ExtensionRegistry extensions(exts, arena);
   TestModel parsed_model = ::hpb::Parse<TestModel>(data, extensions).value();
   EXPECT_TRUE(::hpb::HasExtension(&parsed_model, theme));
 }