[webkit-reviews] review denied: [Bug 212383] for-of should check the iterable is a JSArray for FastArray in DFG iterator_open : [Attachment 400381] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 27 16:40:11 PDT 2020
Saam Barati <sbarati at apple.com> has denied 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 400381: Patch
https://bugs.webkit.org/attachment.cgi?id=400381&action=review
--- Comment #11 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 400381
--> https://bugs.webkit.org/attachment.cgi?id=400381
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=400381&action=review
Mostly LGTM, but I found a bug
> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:488
> +
nit: remove extra new line
> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:530
> + if (structure->typeInfo().type() == ArrayType)
> + return SpecArray;
> + if (structure->typeInfo().type() == StringObjectType)
> + return SpecStringObject;
can you turn all these ifs into a helper lambda, and capture the result JSType,
and then add a debug assert that speculationFromClassInfoInheritance is a
supertype of whatever we return here?
> Source/JavaScriptCore/bytecode/SpeculatedType.h:527
> +// ASSERT(!c->inherits(classInfo) ||
speculationChecked(speculationFromCell(c),
speculationFromClassInfoInheritance(classInfo)));
can you add a form of this assert as I propose above
> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3520
> + ASSERT(classInfo->inheritsJSTypeRange->first <=
constant.asCell()->type() && constant.asCell()->type() <=
classInfo->inheritsJSTypeRange->last);
I still think we should add a helper for this as we're now dealing with a
struct.
Each call site shouldn't have to worry about if the bounds are inclusive or
not, let's abstract it into the helper
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13941
> + LValue hasType = isCellWithType(cell,
classInfo->inheritsJSTypeRange->first, classInfo->inheritsJSTypeRange->last);
You're misunderstanding what this function is doing.
More information about the webkit-reviews
mailing list