[webkit-reviews] review granted: [Bug 215746] [JSC] Add concurrency-aware version of isCallable / isConstructor to make it usable in DFG compiler : [Attachment 407037] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 215746: [JSC] Add concurrency-aware version of isCallable / isConstructor
to make it usable in DFG compiler
https://bugs.webkit.org/show_bug.cgi?id=215746

Attachment 407037: Patch

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




--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 407037
  --> https://bugs.webkit.org/attachment.cgi?id=407037
Patch

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

> Source/JavaScriptCore/runtime/Concurrency.h:34
> +    OnMainThread,
> +    Concurrently,

to make these names match, I'd do:
OnMainThread, OnConcurrentThread

MainThread, ConcurrentThread

Main, Concurrent

> Source/JavaScriptCore/runtime/JSCellInlines.h:254
> +	   else {

Do we need an else here, since previous "if constexpr" ends in a return?

> Source/JavaScriptCore/runtime/JSCellInlines.h:257
> +	       if (type() == InternalFunctionType)

Maybe add a FIXME + link to bug about your plan to when NPAPI is removed b/c
its concurrency issue, and rename getCallData to indicate it can be concurrent?

Or perhaps, if that doesn't work out, add a new getCallDataConcurrently
version?

> Source/JavaScriptCore/runtime/JSCellInlines.h:272
> +    else {

Do we need an else here, since previous "if constexpr" ends in a return?