[webkit-reviews] review granted: [Bug 237306] Add a DeferTraps scope : [Attachment 453509] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 237306: Add a DeferTraps scope
https://bugs.webkit.org/show_bug.cgi?id=237306

Attachment 453509: patch

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




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

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:955
> +	   DeferTraps deferTraps(vm); // We can't jettison this code if we're
about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1020
> +	   DeferTraps deferTraps(vm); // We can't jettison this code if we're
about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1311
> +	   DeferTraps deferTraps(vm); // We can't jettison this code if we're
about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1407
>      // Execute the code:
>      throwScope.release();

You didn't put this ThrowScope::release() in the DeferScope unlike in
`Interpreter::execute(CallFrameClosure& closure)` below.  The same is true for
all Interpreter execute functions above.  Realistically, I don't think we will
or should ever handle traps in ThrowScope::release() because on release builds,
we rely on it being a no-op.  I still think that it is the better idiom to keep
the ThrowScope::release() close to the statement that the release applies to
i.e. the execute.  Also, the code reads more naturally this way.  Regardless,
let's make it consistent every where.

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:91
> +	   DeferTraps deferTraps(vm); // We can't jettison this code if we're
about to run it.

nit: can you add a newline after this decl?