[webkit-reviews] review granted: [Bug 228146] speculateNeitherDoubleNorStringNorHeapBigInt should only have a single JSType branch : [Attachment 433934] Patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 228146: speculateNeitherDoubleNorStringNorHeapBigInt should only have a
single JSType branch
https://bugs.webkit.org/show_bug.cgi?id=228146

Attachment 433934: Patch

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




--- Comment #4 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 433934
  --> https://bugs.webkit.org/attachment.cgi?id=433934
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:7
> +

Maybe add some small comment here explaining your trick for people scanning
through the Changelog.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792
>> +	DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt),
m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType
}));
> 
> It seems like this should be SpecAnyString but maybe I was missing something?

SpecAnyString does not exist, SpecString = SpecStringIdent | SpecStringVar is
the union type.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-19012
> -	   return m_out.equal(

I think I would have kept this version of isType intact, and called it from the
JSTypeRange version when range.last == range.first, but I'm ok with your
approach even if I find it less intuitive.