[webkit-reviews] review granted: [Bug 204555] [JSC] Introduce IsoHeapCellType : [Attachment 384252] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 204555: [JSC] Introduce IsoHeapCellType
https://bugs.webkit.org/show_bug.cgi?id=204555

Attachment 384252: Patch

https://bugs.webkit.org/attachment.cgi?id=384252&action=review




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 384252
  --> https://bugs.webkit.org/attachment.cgi?id=384252
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384252&action=review

Nice work.  r=me with suggestions.

> Source/JavaScriptCore/heap/IsoHeapCellType.h:35
> +class IsoHeapCellType final : public HeapCellType {
> +public:
> +    IsoHeapCellType()
> +	   : HeapCellType(CellAttributes(CellType::needsDestruction ?
NeedsDestruction : DoesNotNeedDestruction, HeapCell::JSCell))

If I understand you correctly, you intend to only use IsoHeapCellType for
destructible types, yes?  Why not:
1. name it IsoHeapDestructibleCellType?
2. Always pass NeedsDestruction down to HeapCellType constructor here?
3. static_assert(CellType::needsDestruction)?

> Source/JavaScriptCore/heap/IsoHeapCellType.h:53
> +	   DestroyFunc()(vm, cell);

Why not just go direct to CellType::destroy(cell) here?