[webkit-reviews] review granted: [Bug 68567] Add custom vtable struct to ClassInfo struct : [Attachment 108542] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted  review:
Bug 68567: Add custom vtable struct to ClassInfo struct
https://bugs.webkit.org/show_bug.cgi?id=68567

Attachment 108542: Patch
https://bugs.webkit.org/attachment.cgi?id=108542&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108542&action=review


r=me with the changes I suggested.

> Source/JavaScriptCore/runtime/ClassInfo.h:38
> +    struct VirtualMethodTable {
> +	   typedef void (*VisitChildrenFunctionPtr)(JSCell*, SlotVisitor&);
> +	   VisitChildrenFunctionPtr visitChildrenFunctionPtr;
> +    };
> +
> +#define CREATE_METHOD_TABLE(ClassName) { \

It is bad when one thing becomes two. You call this thing "virtual method
table" and "method table". (Below, you also call it "vtable".) Let's call it
exactly one thing everywhere. I like "method table".

> Source/JavaScriptCore/runtime/RegExpObject.h:85
>	   virtual void visitChildrenVirtual(SlotVisitor&);
>	   static void visitChildren(JSCell*, SlotVisitor&);

Why did these need to become public? This is the kind of thing that can benefit
from a line-by-line ChangeLog comment. You should add one.