cppgc: Another round of docs

Document:
- Visitor

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

include/cppgc/visitor.h

@@ -14,7 +14,11 @@
 #include "cppgc/trace-trait.h"
 
 namespace cppgc {
+
 namespace internal {
+template <typename T, typename WeaknessPolicy, typename LocationPolicy,
+          typename CheckingPolicy>
+class BasicPersistent;
 class VisitorBase;
 }  // namespace internal
 
@@ -22,10 +26,28 @@ using WeakCallback = void (*)(const LivenessBroker&, const void*);
 
 /**
  * Visitor passed to trace methods. All managed pointers must have called the
- * visitor's trace method on them.
+ * Visitor's trace method on them.
+ *
+ * \code
+ * class Foo final : public GarbageCollected<Foo> {
+ *  public:
+ *   void Trace(Visitor* visitor) const {
+ *     visitor->Trace(foo_);
+ *     visitor->Trace(weak_foo_);
+ *   }
+ *  private:
+ *   Member<Foo> foo_;
+ *   WeakMember<Foo> weak_foo_;
+ * };
+ * \endcode
  */
 class Visitor {
  public:
+  /**
+   * Trace method for Member.
+   *
+   * \param member Member reference retaining an object.
+   */
   template <typename T>
   void Trace(const Member<T>& member) {
     const T* value = member.GetRawAtomic();
@@ -33,11 +55,16 @@ class Visitor {
     Trace(value);
   }
 
+  /**
+   * Trace method for WeakMember.
+   *
+   * \param weak_member WeakMember reference weakly retaining an object.
+   */
   template <typename T>
   void Trace(const WeakMember<T>& weak_member) {
     static_assert(sizeof(T), "Pointee type must be fully defined.");
     static_assert(internal::IsGarbageCollectedType<T>::value,
-                  "T must be GarabgeCollected or GarbageCollectedMixin type");
+                  "T must be GarbageCollected or GarbageCollectedMixin type");
 
     const T* value = weak_member.GetRawAtomic();
 
@@ -52,35 +79,12 @@ class Visitor {
               &HandleWeak<WeakMember<T>>, &weak_member);
   }
 
-  template <typename Persistent,
-            std::enable_if_t<Persistent::IsStrongPersistent::value>* = nullptr>
-  void TraceRoot(const Persistent& p, const SourceLocation& loc) {
-    using PointeeType = typename Persistent::PointeeType;
-    static_assert(sizeof(PointeeType),
-                  "Persistent's pointee type must be fully defined");
-    static_assert(internal::IsGarbageCollectedType<PointeeType>::value,
-                  "Persisent's pointee type must be GarabgeCollected or "
-                  "GarbageCollectedMixin");
-    if (!p.Get()) {
-      return;
-    }
-    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()));
-  }
-
-  template <
-      typename WeakPersistent,
-      std::enable_if_t<!WeakPersistent::IsStrongPersistent::value>* = nullptr>
-  void TraceRoot(const WeakPersistent& p, const SourceLocation& loc) {
-    using PointeeType = typename WeakPersistent::PointeeType;
-    static_assert(sizeof(PointeeType),
-                  "Persistent's pointee type must be fully defined");
-    static_assert(internal::IsGarbageCollectedType<PointeeType>::value,
-                  "Persisent's pointee type must be GarabgeCollected or "
-                  "GarbageCollectedMixin");
-    VisitWeakRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
-                  &HandleWeak<WeakPersistent>, &p);
-  }
-
+  /**
+   * Trace method for inlined objects that are not allocated themselves but
+   * otherwise follow managed heap layout and have a Trace() method.
+   *
+   * \param object reference of the inlined object.
+   */
   template <typename T>
   void Trace(const T& object) {
 #if V8_ENABLE_CHECKS
@@ -93,12 +97,24 @@ class Visitor {
     TraceTrait<T>::Trace(this, &object);
   }
 
+  /**
+   * Registers a weak callback method on the object of type T. See
+   * LivenessBroker for an usage example.
+   *
+   * \param object of type T specifying a weak callback method.
+   */
   template <typename T, void (T::*method)(const LivenessBroker&)>
-  void RegisterWeakCallbackMethod(const T* obj) {
-    RegisterWeakCallback(&WeakCallbackMethodDelegate<T, method>, obj);
+  void RegisterWeakCallbackMethod(const T* object) {
+    RegisterWeakCallback(&WeakCallbackMethodDelegate<T, method>, object);
   }
 
-  virtual void RegisterWeakCallback(WeakCallback, const void*) {}
+  /**
+   * Registers a weak callback that is invoked during garbage collection.
+   *
+   * \param callback to be invoked.
+   * \param data custom data that is passed to the callback.
+   */
+  virtual void RegisterWeakCallback(WeakCallback callback, const void* data) {}
 
  protected:
   virtual void Visit(const void* self, TraceDescriptor) {}
@@ -131,11 +147,40 @@ class Visitor {
 
   Visitor() = default;
 
+  template <typename Persistent,
+            std::enable_if_t<Persistent::IsStrongPersistent::value>* = nullptr>
+  void TraceRoot(const Persistent& p, const SourceLocation& loc) {
+    using PointeeType = typename Persistent::PointeeType;
+    static_assert(sizeof(PointeeType),
+                  "Persistent's pointee type must be fully defined");
+    static_assert(internal::IsGarbageCollectedType<PointeeType>::value,
+                  "Persistent's pointee type must be GarbageCollected or "
+                  "GarbageCollectedMixin");
+    if (!p.Get()) {
+      return;
+    }
+    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()));
+  }
+
+  template <
+      typename WeakPersistent,
+      std::enable_if_t<!WeakPersistent::IsStrongPersistent::value>* = nullptr>
+  void TraceRoot(const WeakPersistent& p, const SourceLocation& loc) {
+    using PointeeType = typename WeakPersistent::PointeeType;
+    static_assert(sizeof(PointeeType),
+                  "Persistent's pointee type must be fully defined");
+    static_assert(internal::IsGarbageCollectedType<PointeeType>::value,
+                  "Persistent's pointee type must be GarbageCollected or "
+                  "GarbageCollectedMixin");
+    VisitWeakRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
+                  &HandleWeak<WeakPersistent>, &p);
+  }
+
   template <typename T>
   void Trace(const T* t) {
     static_assert(sizeof(T), "Pointee type must be fully defined.");
     static_assert(internal::IsGarbageCollectedType<T>::value,
-                  "T must be GarabgeCollected or GarbageCollectedMixin type");
+                  "T must be GarbageCollected or GarbageCollectedMixin type");
     if (!t) {
       return;
     }
@@ -147,6 +192,9 @@ class Visitor {
 #endif  // V8_ENABLE_CHECKS
 
   friend class internal::VisitorBase;
+  template <typename T, typename WeaknessPolicy, typename LocationPolicy,
+            typename CheckingPolicy>
+  friend class internal::BasicPersistent;
 };
 
 }  // namespace cppgc

src/heap/cppgc/visitor.h

@@ -5,6 +5,7 @@
 #ifndef V8_HEAP_CPPGC_VISITOR_H_
 #define V8_HEAP_CPPGC_VISITOR_H_
 
+#include "include/cppgc/persistent.h"
 #include "include/cppgc/visitor.h"
 
 namespace cppgc {
@@ -15,6 +16,17 @@ namespace internal {
 class VisitorBase : public cppgc::Visitor {
  public:
   VisitorBase() = default;
+
+  template <typename T>
+  void TraceRootForTesting(const Persistent<T>& p, const SourceLocation& loc) {
+    TraceRoot(p, loc);
+  }
+
+  template <typename T>
+  void TraceRootForTesting(const WeakPersistent<T>& p,
+                           const SourceLocation& loc) {
+    TraceRoot(p, loc);
+  }
 };
 
 }  // namespace internal

test/unittests/heap/cppgc/marking-visitor-unittest.cc

@@ -88,7 +88,7 @@ TEST_F(MarkingVisitorTest, MarkPersistent) {
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRoot(object, SourceLocation::Current());
+  visitor.TraceRootForTesting(object, SourceLocation::Current());
 
   EXPECT_TRUE(header.IsMarked());
 }
@@ -102,7 +102,7 @@ TEST_F(MarkingVisitorTest, MarkPersistentMixin) {
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRoot(mixin, SourceLocation::Current());
+  visitor.TraceRootForTesting(mixin, SourceLocation::Current());
 
   EXPECT_TRUE(header.IsMarked());
 }
@@ -144,7 +144,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistent) {
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRoot(object, SourceLocation::Current());
+  visitor.TraceRootForTesting(object, SourceLocation::Current());
 
   EXPECT_FALSE(header.IsMarked());
 }
@@ -158,7 +158,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixin) {
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRoot(mixin, SourceLocation::Current());
+  visitor.TraceRootForTesting(mixin, SourceLocation::Current());
 
   EXPECT_FALSE(header.IsMarked());
 }
@@ -248,7 +248,7 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
             Persistent<GCedWithInConstructionCallback> object(obj);
-            visitor.TraceRoot(object, SourceLocation::Current());
+            visitor.TraceRootForTesting(object, SourceLocation::Current());
           });
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
@@ -259,7 +259,7 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
             Persistent<MixinWithInConstructionCallback> mixin(obj);
-            visitor.TraceRoot(mixin, SourceLocation::Current());
+            visitor.TraceRootForTesting(mixin, SourceLocation::Current());
           });
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
@@ -270,7 +270,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentInConstruction) {
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
             WeakPersistent<GCedWithInConstructionCallback> object(obj);
-            visitor.TraceRoot(object, SourceLocation::Current());
+            visitor.TraceRootForTesting(object, SourceLocation::Current());
           });
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
@@ -281,7 +281,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixinInConstruction) {
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
             WeakPersistent<MixinWithInConstructionCallback> mixin(obj);
-            visitor.TraceRoot(mixin, SourceLocation::Current());
+            visitor.TraceRootForTesting(mixin, SourceLocation::Current());
           });
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }