[webkit-reviews] review granted: [Bug 212383] for-of should check the iterable is a JSArray for FastArray in DFG iterator_open : [Attachment 400423] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 212383: for-of should check the iterable is a JSArray for FastArray in DFG
iterator_open
https://bugs.webkit.org/show_bug.cgi?id=212383

Attachment 400423: Patch

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




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

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

r=me

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:488
> +

extra space

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:491
> +    if (classInfo->isSubClassOf(JSString::info()))
> +	   return SpecString;

this can still be an == with an assert that if == is false then isSubClassOf is
also false

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:536
> +    SpeculatedType classInfoTypes =
speculationFromClassInfoInheritance(structure->classInfo());
> +    ASSERT(!filteredResult || isSubtypeSpeculation(filteredResult,
classInfoTypes));

I wouldn't call this unconditionally. Just branch on filtered result, and if
non zero, debug assert.

I suspect the compiler isn't smart enough to not do this function invocation
when !!filteredResult

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:538
> +    // Fallback to what ClassInfo tells us if we don't have something more
refined.

remove this comment, it's obvious from reading the code

> Source/JavaScriptCore/jit/AssemblyHelpers.h:998
> +	   return branchIfType(cellGPR, JSTypeRange { type, type });

why not just make JSTypeRange have a constructor from JSType,  and then you
wouldn't need this variant anymore?