Devirtualize cel::Data

common/data.h

@@ -15,6 +15,7 @@
 #ifndef THIRD_PARTY_CEL_CPP_COMMON_DATA_H_
 #define THIRD_PARTY_CEL_CPP_COMMON_DATA_H_
 
+#include <cstddef>
 #include <cstdint>
 
 #include "absl/base/nullability.h"
@@ -34,22 +35,25 @@ namespace common_internal {
 
 class ReferenceCount;
 
-void SetDataReferenceCount(
-    absl::Nonnull<const Data*> data,
-    absl::Nonnull<const ReferenceCount*> refcount) noexcept;
+void SetDataReferenceCount(absl::Nonnull<const Data*> data,
+                           absl::Nonnull<const ReferenceCount*> refcount);
 
 absl::Nullable<const ReferenceCount*> GetDataReferenceCount(
-    absl::Nonnull<const Data*> data) noexcept;
+    absl::Nonnull<const Data*> data);
 
 }  // namespace common_internal
 
 // `Data` is one of the base classes of objects that can be managed by
 // `MemoryManager`, the other is `google::protobuf::MessageLite`.
 class Data {
  public:
-  virtual ~Data() = default;
+  Data(const Data&) = default;
+  Data(Data&&) = default;
+  ~Data() = default;
+  Data& operator=(const Data&) = default;
+  Data& operator=(Data&&) = default;
 
-  absl::Nullable<google::protobuf::Arena*> GetArena() const noexcept {
+  absl::Nullable<google::protobuf::Arena*> GetArena() const {
     return (owner_ & kOwnerBits) == kOwnerArenaBit
                ? reinterpret_cast<google::protobuf::Arena*>(owner_ & kOwnerPointerMask)
                : nullptr;
@@ -61,14 +65,11 @@ class Data {
   // reference count ahead of time and then update it with the data it has to
   // delete, but that is a bit counter intuitive. Doing it this way is also
   // similar to how std::enable_shared_from_this works.
-  Data() noexcept : Data(nullptr) {}
+  Data() = default;
 
-  Data(const Data&) = default;
-  Data(Data&&) = default;
-  Data& operator=(const Data&) = default;
-  Data& operator=(Data&&) = default;
+  Data(std::nullptr_t) = delete;
 
-  explicit Data(absl::Nullable<google::protobuf::Arena*> arena) noexcept
+  explicit Data(absl::Nullable<google::protobuf::Arena*> arena)
       : owner_(reinterpret_cast<uintptr_t>(arena) |
                (arena != nullptr ? kOwnerArenaBit : kOwnerNone)) {}
 
@@ -84,10 +85,9 @@ class Data {
 
   friend void common_internal::SetDataReferenceCount(
       absl::Nonnull<const Data*> data,
-      absl::Nonnull<const common_internal::ReferenceCount*> refcount) noexcept;
+      absl::Nonnull<const common_internal::ReferenceCount*> refcount);
   friend absl::Nullable<const common_internal::ReferenceCount*>
-  common_internal::GetDataReferenceCount(
-      absl::Nonnull<const Data*> data) noexcept;
+  common_internal::GetDataReferenceCount(absl::Nonnull<const Data*> data);
   template <typename T>
   friend struct Ownable;
   template <typename T>
@@ -100,14 +100,14 @@ namespace common_internal {
 
 inline void SetDataReferenceCount(
     absl::Nonnull<const Data*> data,
-    absl::Nonnull<const ReferenceCount*> refcount) noexcept {
+    absl::Nonnull<const ReferenceCount*> refcount) {
   ABSL_DCHECK_EQ(data->owner_, Data::kOwnerNone);
   data->owner_ =
       reinterpret_cast<uintptr_t>(refcount) | Data::kOwnerReferenceCountBit;
 }
 
 inline absl::Nullable<const ReferenceCount*> GetDataReferenceCount(
-    absl::Nonnull<const Data*> data) noexcept {
+    absl::Nonnull<const Data*> data) {
   return (data->owner_ & Data::kOwnerBits) == Data::kOwnerReferenceCountBit
              ? reinterpret_cast<const ReferenceCount*>(data->owner_ &
                                                        Data::kOwnerPointerMask)

common/internal/reference_count.cc

@@ -31,27 +31,27 @@
 namespace cel::common_internal {
 
 template class DeletingReferenceCount<google::protobuf::MessageLite>;
-template class DeletingReferenceCount<Data>;
 
 namespace {
 
 class ReferenceCountedStdString final : public ReferenceCounted {
  public:
+  static std::pair<absl::Nonnull<const ReferenceCount*>, absl::string_view> New(
+      std::string&& string) {
+    const auto* const refcount =
+        new ReferenceCountedStdString(std::move(string));
+    const auto* const refcount_string = std::launder(
+        reinterpret_cast<const std::string*>(&refcount->string_[0]));
+    return std::pair{
+        static_cast<absl::Nonnull<const ReferenceCount*>>(refcount),
+        absl::string_view(*refcount_string)};
+  }
+
   explicit ReferenceCountedStdString(std::string&& string) {
     (::new (static_cast<void*>(&string_[0])) std::string(std::move(string)))
         ->shrink_to_fit();
   }
 
-  const char* data() const noexcept {
-    return std::launder(reinterpret_cast<const std::string*>(&string_[0]))
-        ->data();
-  }
-
-  size_t size() const noexcept {
-    return std::launder(reinterpret_cast<const std::string*>(&string_[0]))
-        ->size();
-  }
-
  private:
   void Finalize() noexcept override {
     std::destroy_at(std::launder(reinterpret_cast<std::string*>(&string_[0])));
@@ -60,6 +60,19 @@ class ReferenceCountedStdString final : public ReferenceCounted {
   alignas(std::string) char string_[sizeof(std::string)];
 };
 
+class ReferenceCountedString final : public ReferenceCounted {
+ public:
+  static std::pair<absl::Nonnull<const ReferenceCount*>, absl::string_view> New(
+      absl::string_view string) {
+    const auto* const refcount =
+        ::new (internal::New(Overhead() + string.size()))
+            ReferenceCountedString(string);
+    return std::pair{
+        static_cast<absl::Nonnull<const ReferenceCount*>>(refcount),
+        absl::string_view(refcount->data_, refcount->size_)};
+  }
+
+ private:
 // ReferenceCountedString is non-standard-layout due to having virtual functions
 // from a base class. This causes compilers to warn about the use of offsetof(),
 // but it still works here, so silence the warning and proceed.
@@ -68,54 +81,40 @@ class ReferenceCountedStdString final : public ReferenceCounted {
 #pragma GCC diagnostic ignored "-Winvalid-offsetof"
 #endif
 
-class ReferenceCountedString final : public ReferenceCounted {
- public:
-  static const ReferenceCountedString* New(const char* data, size_t size) {
-    return ::new (internal::New(offsetof(ReferenceCountedString, data_) + size))
-        ReferenceCountedString(size, data);
-  }
-
-  const char* data() const noexcept { return data_; }
+  static size_t Overhead() { return offsetof(ReferenceCountedString, data_); }
 
-  size_t size() const noexcept { return size_; }
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
 
- private:
-  ReferenceCountedString(size_t size, const char* data) noexcept : size_(size) {
-    std::memcpy(data_, data, size);
+  explicit ReferenceCountedString(absl::string_view string)
+      : size_(string.size()) {
+    std::memcpy(data_, string.data(), size_);
   }
 
   void Delete() noexcept override {
     void* const that = this;
     const auto size = size_;
     std::destroy_at(this);
-    internal::SizedDelete(that, offsetof(ReferenceCountedString, data_) + size);
+    internal::SizedDelete(that, Overhead() + size);
   }
 
   const size_t size_;
   char data_[];
 };
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 }  // namespace
 
 std::pair<absl::Nonnull<const ReferenceCount*>, absl::string_view>
 MakeReferenceCountedString(absl::string_view value) {
   ABSL_DCHECK(!value.empty());
-  const auto* refcount =
-      ReferenceCountedString::New(value.data(), value.size());
-  return std::pair{refcount,
-                   absl::string_view(refcount->data(), refcount->size())};
+  return ReferenceCountedString::New(value);
 }
 
 std::pair<absl::Nonnull<const ReferenceCount*>, absl::string_view>
 MakeReferenceCountedString(std::string&& value) {
   ABSL_DCHECK(!value.empty());
-  const auto* refcount = new ReferenceCountedStdString(std::move(value));
-  return std::pair{refcount,
-                   absl::string_view(refcount->data(), refcount->size())};
+  return ReferenceCountedStdString::New(std::move(value));
 }
 
 }  // namespace cel::common_internal

common/internal/reference_count.h

@@ -174,6 +174,7 @@ class EmplacedReferenceCount final : public ReferenceCounted {
   static_assert(!std::is_reference_v<T>, "T must not be a reference");
   static_assert(!std::is_volatile_v<T>, "T must not be volatile qualified");
   static_assert(!std::is_const_v<T>, "T must not be const qualified");
+  static_assert(!std::is_array_v<T>, "T must not be an array");
 
   template <typename... Args>
   explicit EmplacedReferenceCount(T*& value, Args&&... args) noexcept(
@@ -184,7 +185,7 @@ class EmplacedReferenceCount final : public ReferenceCounted {
 
  private:
   void Finalize() noexcept override {
-    std::launder(reinterpret_cast<T*>(&value_[0]))->~T();
+    std::destroy_at(std::launder(reinterpret_cast<T*>(&value_[0])));
   }
 
   // We store the instance of `T` in a char buffer and use placement new and
@@ -205,15 +206,12 @@ class DeletingReferenceCount final : public ReferenceCounted {
       : to_delete_(to_delete) {}
 
  private:
-  void Finalize() noexcept override {
-    delete std::exchange(to_delete_, nullptr);
-  }
+  void Finalize() noexcept override { delete to_delete_; }
 
-  const T* to_delete_;
+  absl::Nonnull<const T*> const to_delete_;
 };
 
 extern template class DeletingReferenceCount<google::protobuf::MessageLite>;
-extern template class DeletingReferenceCount<Data>;
 
 template <typename T>
 absl::Nonnull<const ReferenceCount*> MakeDeletingReferenceCount(
@@ -223,12 +221,12 @@ absl::Nonnull<const ReferenceCount*> MakeDeletingReferenceCount(
   }
   if constexpr (std::is_base_of_v<google::protobuf::MessageLite, T>) {
     return new DeletingReferenceCount<google::protobuf::MessageLite>(to_delete);
-  } else if constexpr (std::is_base_of_v<Data, T>) {
-    auto* refcount = new DeletingReferenceCount<Data>(to_delete);
-    common_internal::SetDataReferenceCount(to_delete, refcount);
-    return refcount;
   } else {
-    return new DeletingReferenceCount<T>(to_delete);
+    auto* refcount = new DeletingReferenceCount<T>(to_delete);
+    if constexpr (std::is_base_of_v<Data, T>) {
+      common_internal::SetDataReferenceCount(to_delete, refcount);
+    }
+    return refcount;
   }
 }
 

common/internal/reference_count_test.cc

@@ -99,8 +99,9 @@ struct OtherObject final {
 TEST(DeletingReferenceCount, Data) {
   auto* data = new DataObject();
   const auto* refcount = MakeDeletingReferenceCount(data);
-  EXPECT_THAT(refcount, WhenDynamicCastTo<const DeletingReferenceCount<Data>*>(
-                            NotNull()));
+  EXPECT_THAT(
+      refcount,
+      WhenDynamicCastTo<const DeletingReferenceCount<DataObject>*>(NotNull()));
   EXPECT_EQ(common_internal::GetDataReferenceCount(data), refcount);
   StrongUnref(refcount);
 }