cppgc: Refine Member a bit

This change comprises a few tiny changes wrt Member:
1) Move member policies to a separate file so that some of them
(CheckingPolicy) can be reused by Persistent;
2) SFINAE out incompatible pointers from heterogeneous ctor/asgnmt;
3) Rename kMemberSentinel to kSentinelPointer.

Bug: chromium:1056170
Change-Id: I4482998e6ba61005a5d0861dcae9fab2aa43702c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2139587
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67061}

BUILD.gn

@@ -3951,6 +3951,7 @@ v8_source_set("cppgc_base") {
     "include/cppgc/internal/api-contants.h",
     "include/cppgc/internal/finalizer-traits.h",
     "include/cppgc/internal/gc-info.h",
+    "include/cppgc/internal/pointer-policies.h",
     "include/cppgc/member.h",
     "include/cppgc/platform.h",
     "include/cppgc/source-location.h",
@@ -3968,8 +3969,8 @@ v8_source_set("cppgc_base") {
     "src/heap/cppgc/heap-object-header.h",
     "src/heap/cppgc/heap.cc",
     "src/heap/cppgc/heap.h",
-    "src/heap/cppgc/member.cc",
     "src/heap/cppgc/platform.cc",
+    "src/heap/cppgc/pointer-policies.cc",
     "src/heap/cppgc/sanitizers.h",
     "src/heap/cppgc/source-location.cc",
     "src/heap/cppgc/stack.cc",

include/cppgc/internal/pointer-policies.h

@@ -0,0 +1,74 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef INCLUDE_CPPGC_INTERNAL_POINTER_POLICIES_H_
+#define INCLUDE_CPPGC_INTERNAL_POINTER_POLICIES_H_
+
+#include <cstdint>
+
+#include "include/v8config.h"
+
+namespace cppgc {
+namespace internal {
+
+// Tags to distinguish between strong and weak member types.
+class StrongMemberTag;
+class WeakMemberTag;
+class UntracedMemberTag;
+
+struct DijkstraWriteBarrierPolicy {
+  static void InitializingBarrier(const void*, const void*) {
+    // Since in initializing writes the source object is always white, having no
+    // barrier doesn't break the tri-color invariant.
+  }
+  static void AssigningBarrier(const void*, const void*) {
+    // TODO(chromium:1056170): Add actual implementation.
+  }
+};
+
+struct NoWriteBarrierPolicy {
+  static void InitializingBarrier(const void*, const void*) {}
+  static void AssigningBarrier(const void*, const void*) {}
+};
+
+class V8_EXPORT EnabledCheckingPolicy {
+ protected:
+  EnabledCheckingPolicy();
+  void CheckPointer(const void* ptr);
+
+ private:
+  void* impl_;
+};
+
+class DisabledCheckingPolicy {
+ protected:
+  void CheckPointer(const void* raw) {}
+};
+
+#if V8_ENABLE_CHECKS
+using DefaultCheckingPolicy = EnabledCheckingPolicy;
+#else
+using DefaultCheckingPolicy = DisabledCheckingPolicy;
+#endif
+
+// Special tag type used to denote some sentinel member. The semantics of the
+// sentinel is defined by the embedder.
+struct SentinelPointer {
+  template <typename T>
+  operator T*() const {  // NOLINT
+    static constexpr intptr_t kSentinelValue = -1;
+    return reinterpret_cast<T*>(kSentinelValue);
+  }
+  // Hidden friends.
+  friend bool operator==(SentinelPointer, SentinelPointer) { return true; }
+  friend bool operator!=(SentinelPointer, SentinelPointer) { return false; }
+};
+
+}  // namespace internal
+
+constexpr internal::SentinelPointer kSentinelPointer;
+
+}  // namespace cppgc
+
+#endif  // INCLUDE_CPPGC_INTERNAL_POINTER_POLICIES_H_

include/cppgc/member.h

@@ -7,7 +7,10 @@
 
 #include <atomic>
 #include <cstddef>
+#include <type_traits>
 
+#include "include/cppgc/internal/pointer-policies.h"
+#include "include/cppgc/type-traits.h"
 #include "include/v8config.h"
 
 namespace cppgc {
@@ -16,66 +19,24 @@ class Visitor;
 
 namespace internal {
 
-struct DijkstraWriteBarrierPolicy {
-  static void InitializingBarrier(const void*, const void*) {
-    // Since in initializing writes the source object is always white, having no
-    // barrier doesn't break the tri-color invariant.
-  }
-  static void AssigningBarrier(const void*, const void*) {
-    // TODO(chromium:1056170): Add actual implementation.
-  }
-};
-
-struct NoWriteBarrierPolicy {
-  static void InitializingBarrier(const void*, const void*) {}
-  static void AssigningBarrier(const void*, const void*) {}
-};
-
-class V8_EXPORT EnabledCheckingPolicy {
- public:
-  EnabledCheckingPolicy();
-  void CheckPointer(const void* ptr);
-
- private:
-  void* impl_;
-};
-
-class DisabledCheckingPolicy {
- public:
-  void CheckPointer(const void* raw) {}
-};
-
-#if V8_ENABLE_CHECKS
-using DefaultCheckingPolicy = EnabledCheckingPolicy;
-#else
-using DefaultCheckingPolicy = DisabledCheckingPolicy;
-#endif
-
-// Special tag type used to denote some sentinel member. The semantics of the
-// sentinel is defined by the embedder.
-struct MemberSentinel {};
-constexpr MemberSentinel kMemberSentinel;
-
 // The basic class from which all Member classes are 'generated'.
-template <typename T, class WeaknessTag, class WriteBarrierPolicy,
+template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
-          class CheckingPolicy = DefaultCheckingPolicy>
+          typename CheckingPolicy = DefaultCheckingPolicy>
 class BasicMember : private CheckingPolicy {
  public:
   constexpr BasicMember() = default;
   constexpr BasicMember(std::nullptr_t) {}  // NOLINT
-  constexpr BasicMember(MemberSentinel)     // NOLINT
+  BasicMember(SentinelPointer s) : raw_(s) {}  // NOLINT
-      : raw_(reinterpret_cast<T*>(kSentinelValue)) {}
   BasicMember(T* raw) : raw_(raw) {  // NOLINT
     InitializingWriteBarrier();
     this->CheckPointer(raw_);
   }
-  // TODO(chromium:1056170): Unfortunately, this overload is used ubiquitously
-  // in Blink. Reeavalute the possibility to remove it.
   BasicMember(T& raw) : BasicMember(&raw) {}  // NOLINT
   BasicMember(const BasicMember& other) : BasicMember(other.Get()) {}
   // Allow heterogeneous construction.
   template <typename U, typename OtherBarrierPolicy, typename OtherWeaknessTag,
-            typename OtherCheckingPolicy>
+            typename OtherCheckingPolicy,
+            typename = std::enable_if_t<std::is_base_of<T, U>::value>>
   BasicMember(const BasicMember<U, OtherWeaknessTag, OtherBarrierPolicy,
                                 OtherCheckingPolicy>& other)
       : BasicMember(other.Get()) {}
@@ -85,7 +46,8 @@ class BasicMember : private CheckingPolicy {
   }
   // Allow heterogeneous assignment.
   template <typename U, typename OtherWeaknessTag, typename OtherBarrierPolicy,
-            typename OtherCheckingPolicy>
+            typename OtherCheckingPolicy,
+            typename = std::enable_if_t<std::is_base_of<T, U>::value>>
   BasicMember& operator=(
       const BasicMember<U, OtherWeaknessTag, OtherBarrierPolicy,
                         OtherCheckingPolicy>& other) {
@@ -101,14 +63,14 @@ class BasicMember : private CheckingPolicy {
     Clear();
     return *this;
   }
-  BasicMember& operator=(MemberSentinel) {
+  BasicMember& operator=(SentinelPointer s) {
-    SetRawAtomic(reinterpret_cast<T*>(kSentinelValue));
+    SetRawAtomic(s);
     return *this;
   }
 
-  template <typename U, typename OtherWeaknessTag, typename OtherBarrierPolicy,
+  template <typename OtherWeaknessTag, typename OtherBarrierPolicy,
             typename OtherCheckingPolicy>
-  void Swap(BasicMember<U, OtherWeaknessTag, OtherBarrierPolicy,
+  void Swap(BasicMember<T, OtherWeaknessTag, OtherBarrierPolicy,
                         OtherCheckingPolicy>& other) {
     T* tmp = Get();
     *this = other;
@@ -134,9 +96,6 @@ class BasicMember : private CheckingPolicy {
   }
 
  private:
-  // Must not be odr-used.
-  static constexpr intptr_t kSentinelValue = -1;
-
   void SetRawAtomic(T* raw) {
     reinterpret_cast<std::atomic<T*>*>(&raw_)->store(raw,
                                                      std::memory_order_relaxed);
@@ -180,12 +139,10 @@ bool operator!=(
   return !(member1 == member2);
 }
 
-template <typename T, typename WriteBarrierPolicy>
+template <typename T, typename WriteBarrierPolicy, typename CheckingPolicy>
-using BasicStrongMember =
+struct IsWeak<
-    BasicMember<T, class StrongMemberTag, WriteBarrierPolicy>;
+    internal::BasicMember<T, WeakMemberTag, WriteBarrierPolicy, CheckingPolicy>>
-
+    : std::true_type {};
-template <typename T, typename WriteBarrierPolicy>
-using BasicWeakMember = BasicMember<T, class WeakMemberTag, WriteBarrierPolicy>;
 
 }  // namespace internal
 
@@ -193,8 +150,8 @@ using BasicWeakMember = BasicMember<T, class WeakMemberTag, WriteBarrierPolicy>;
 // collected objects. All Member fields of a class must be traced in the class'
 // trace method.
 template <typename T>
-using Member =
+using Member = internal::BasicMember<T, internal::StrongMemberTag,
-    internal::BasicStrongMember<T, internal::DijkstraWriteBarrierPolicy>;
+                                     internal::DijkstraWriteBarrierPolicy>;
 
 // WeakMember is similar to Member in that it is used to point to other garbage
 // collected objects. However instead of creating a strong pointer to the
@@ -203,15 +160,15 @@ using Member =
 // the object will be garbage collected. At the time of GC the weak pointers
 // will automatically be set to null.
 template <typename T>
-using WeakMember =
+using WeakMember = internal::BasicMember<T, internal::WeakMemberTag,
-    internal::BasicWeakMember<T, internal::DijkstraWriteBarrierPolicy>;
+                                         internal::DijkstraWriteBarrierPolicy>;
 
 // UntracedMember is a pointer to an on-heap object that is not traced for some
 // reason. Do not use this unless you know what you are doing. Keeping raw
 // pointers to on-heap objects is prohibited unless used from stack. Pointee
 // must be kept alive through other means.
 template <typename T>
-using UntracedMember = internal::BasicMember<T, class UntracedMemberTag,
+using UntracedMember = internal::BasicMember<T, internal::UntracedMemberTag,
                                              internal::NoWriteBarrierPolicy>;
 
 }  // namespace cppgc

include/cppgc/type-traits.h

@@ -7,8 +7,6 @@
 
 #include <type_traits>
 
-#include "include/cppgc/member.h"
-
 namespace cppgc {
 
 class Visitor;
@@ -27,10 +25,6 @@ using void_t = typename make_void<Ts...>::type;
 template <typename T>
 struct IsWeak : std::false_type {};
 
-template <typename T, typename WriteBarrierPolicy>
-struct IsWeak<internal::BasicWeakMember<T, WriteBarrierPolicy>>
-    : std::true_type {};
-
 template <typename T, template <typename... V> class U>
 struct IsSubclassOfTemplate {
  private:

src/heap/cppgc/pointer-policies.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "include/cppgc/member.h"
+#include "include/cppgc/internal/pointer-policies.h"
 
 #include "src/base/macros.h"
 

test/unittests/heap/cppgc/member_unittests.cc

@@ -39,7 +39,7 @@ size_t CustomWriteBarrierPolicy::InitializingWriteBarriersTriggered = 0;
 size_t CustomWriteBarrierPolicy::AssigningWriteBarriersTriggered = 0;
 
 using MemberWithCustomBarrier =
-    BasicStrongMember<GCed, CustomWriteBarrierPolicy>;
+    BasicMember<GCed, StrongMemberTag, CustomWriteBarrierPolicy>;
 
 struct CustomCheckingPolicy {
   static std::vector<GCed*> Cached;
@@ -53,8 +53,8 @@ std::vector<GCed*> CustomCheckingPolicy::Cached;
 size_t CustomCheckingPolicy::ChecksTriggered = 0;
 
 using MemberWithCustomChecking =
-    internal::BasicMember<GCed, class StrongMemberTag,
+    BasicMember<GCed, StrongMemberTag, DijkstraWriteBarrierPolicy,
-                          DijkstraWriteBarrierPolicy, CustomCheckingPolicy>;
+                CustomCheckingPolicy>;
 
 class MemberTest : public testing::TestSupportingAllocationOnly {};
 
@@ -227,8 +227,9 @@ TEST_F(MemberTest, WriteBarrierTriggered) {
   // No initializing barriers for std::nullptr_t.
   EXPECT_EQ(1u, CustomWriteBarrierPolicy::InitializingWriteBarriersTriggered);
   EXPECT_EQ(1u, CustomWriteBarrierPolicy::AssigningWriteBarriersTriggered);
-  member2 = kMemberSentinel;
+  member2 = kSentinelPointer;
-  // No initializing barriers for member sentinel.
+  EXPECT_EQ(kSentinelPointer, member2.Get());
+  // No initializing barriers for pointer sentinel.
   EXPECT_EQ(1u, CustomWriteBarrierPolicy::InitializingWriteBarriersTriggered);
   EXPECT_EQ(1u, CustomWriteBarrierPolicy::AssigningWriteBarriersTriggered);
   member2.Swap(member1);