From 763253e2e18209860b667d0eaf035efb0dfc0c15 Mon Sep 17 00:00:00 2001 From: Hong Shin Date: Tue, 22 Oct 2024 17:33:17 -0700 Subject: [PATCH] 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: 688749016 --- hpb/extension.h | 34 ++++++++++----------------- hpb/internal/message_lock_test.cc | 16 +++++++++---- hpb_generator/tests/test_generated.cc | 14 ++++++++--- 3 files changed, 35 insertions(+), 29 deletions(-) diff --git a/hpb/extension.h b/hpb/extension.h index 100296dfab88..01f0214cf4b8 100644 --- 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,24 @@ class ExtensionMiniTableProvider { // then "bar" will be defined in C++ as: // ExtensionIdentifier bar(&namespace_bar_ext); template -class ExtensionIdentifier : public ExtensionMiniTableProvider { +class ExtensionIdentifier { public: using Extension = ExtensionType; using Extendee = ExtendeeType; + 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 +71,12 @@ upb_ExtensionRegistry* GetUpbExtensions( class ExtensionRegistry { public: ExtensionRegistry( - const std::vector& - extensions, + const std::vector& 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 index 5f44231f660f..5bcab5bd8ff7 100644 --- a/hpb/internal/message_lock_test.cc +++ b/hpb/internal/message_lock_test.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -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,20 @@ TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceBothLazy) { TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceOneLazyOneEager) { ::upb::Arena arena; - TestConcurrentExtensionAccess({{&theme}, arena}); - TestConcurrentExtensionAccess({{&ThemeExtension::theme_extension}, arena}); + std::vector e1{}; + e1.emplace_back(theme.mini_table_ext()); + TestConcurrentExtensionAccess({e1, arena}); + std::vector e2{}; + e2.emplace_back(ThemeExtension::theme_extension.mini_table_ext()); + TestConcurrentExtensionAccess({e2, arena}); } TEST(CppGeneratedCode, ConcurrentAccessDoesNotRaceBothEager) { ::upb::Arena arena; - TestConcurrentExtensionAccess( - {{&theme, &ThemeExtension::theme_extension}, arena}); + std::vector exts{}; + exts.emplace_back(theme.mini_table_ext()); + exts.emplace_back(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 index 90ddc99410fa..80d24ef3f22d 100644 --- 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,12 @@ 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 exts{}; + exts.emplace_back(theme.mini_table_ext()); + exts.emplace_back(other_ext.mini_table_ext()); + exts.emplace_back(ThemeExtension::theme_extension.mini_table_ext()); + + ::hpb::ExtensionRegistry extensions(exts, arena); TestModel parsed_model = ::hpb::Parse(bytes.value(), extensions).value(); EXPECT_EQ("Test123", parsed_model.str1()); @@ -1247,7 +1253,9 @@ TEST(CppGeneratedCode, HasExtensionAndRegistry) { std::string data = std::string(::hpb::Serialize(&source, arena).value()); // Test with ExtensionRegistry - ::hpb::ExtensionRegistry extensions({&theme}, arena); + std::vector exts{}; + exts.emplace_back(theme.mini_table_ext()); + ::hpb::ExtensionRegistry extensions(exts, arena); TestModel parsed_model = ::hpb::Parse(data, extensions).value(); EXPECT_TRUE(::hpb::HasExtension(&parsed_model, theme)); }