[webkit-reviews] review denied: [Bug 182470] [ESNext][BigInt] We should add support to BigInt into speculation : [Attachment 333571] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 13 15:27:30 PST 2018


Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 182470: [ESNext][BigInt] We should add support to BigInt into speculation
https://bugs.webkit.org/show_bug.cgi?id=182470

Attachment 333571: Patch

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




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

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

r- just because of a few comments, but the patch LGTM overall. It'd be good to
have some inferred type related tests.

> Source/JavaScriptCore/ChangeLog:3
> +	   [ESNext][BigInt] We should add support to BigInt into speculation

"We should add support to BigInt into speculation" => "Add support for BigInt
in SpeculatedType"

> Source/JavaScriptCore/ChangeLog:8
> +	   This patch is introducing SpecBigInt type to DFG to enable BigInt

is introducing => introduces the

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10755
> +	   case InferredType::BigInt:
> +	       speculate(BadType, jsValueValue(value), edge.node(),
isNotCell(value, provenType(edge)));
> +	       speculate(BadType, jsValueValue(value), edge.node(),
isNotBigInt(value, provenType(edge)));
> +	       return;

Do you have a test for this?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14904
> +

You also need to add support in:
void speculate(Edge edge)

Should lead to compile time a crash, so you should add code that tests this.

> Source/JavaScriptCore/runtime/JSBigInt.h:40
> +    static const unsigned StructureFlags = Base::StructureFlags |
OverridesToThis;

Why? You're not overriding it. Should you be?


More information about the webkit-reviews mailing list