[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