[webkit-reviews] review granted: [Bug 214539] Simplify DisallowScope, DisallowGC, and DisallowVMReentry implementations. : [Attachment 404887] proposed patch.

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 214539: Simplify DisallowScope, DisallowGC, and DisallowVMReentry
implementations.
https://bugs.webkit.org/show_bug.cgi?id=214539

Attachment 404887: proposed patch.

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




--- Comment #20 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 404887
  --> https://bugs.webkit.org/attachment.cgi?id=404887
proposed patch.

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

r=me with nits.

> Source/JavaScriptCore/ChangeLog:28
> +	      If Options::crashOnDisallowedVMEntry() is false, an attempt to
call into the VM
> +	      while disallowed will return immediately with an undefined result
without
> +	      invoking any script.

Is this better than throwing an Exception of some kind?

> Source/JavaScriptCore/runtime/DisallowScope.h:42
> +	   auto count = T::scopeReentryCount();
> +	   T::setScopeReentryCount(++count);

Why not make the API a ref and deref? Seems like you need two functions either
way?

> Source/JavaScriptCore/runtime/DisallowVMEntry.h:35
> +    WTF_MAKE_NONCOPYABLE(DisallowVMReentry);

Nit: I don't think you need this because DisallowScope isn't copyable.

> Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:65
> +	   m_disallowGC.reset();
> +	   m_disallowVMEntry.reset();

Unrelated but its weird that this is called reset... I would have expected
clear() but ok.