[webkit-reviews] review granted: [Bug 178726] [JSC] Introduce @toObject : [Attachment 324690] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 27 11:29:40 PDT 2017
Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 178726: [JSC] Introduce @toObject
https://bugs.webkit.org/show_bug.cgi?id=178726
Attachment 324690: Patch
https://bugs.webkit.org/attachment.cgi?id=324690&action=review
--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 324690
--> https://bugs.webkit.org/attachment.cgi?id=324690
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=324690&action=review
r=me with comments
> Source/JavaScriptCore/ChangeLog:9
> + Previously we emulated @toObject behavior in builtin JS. But it
consumes much bytecode size while @toObject
consumes much => consumes a lot
> Source/JavaScriptCore/builtins/ArrayConstructor.js:55
> + var arrayLike = @toObject(items, "Array.from requires an array-like
object - not null or undefined");
why is this OK? `items` us used in the code below so this seems like it changed
behavior
> Source/JavaScriptCore/builtins/DatePrototype.js:43
> + options = @toObject(opts, "");
Why switch this? We know opts is not null/undefined here
> Source/JavaScriptCore/builtins/DatePrototype.js:100
> + options = @toObject(opts, "");
Why switch this? We know opts is not null/undefined here
> Source/JavaScriptCore/builtins/DatePrototype.js:150
> + options = @toObject(opts, "");
Why switch this? We know opts is not null/undefined here
> Source/JavaScriptCore/builtins/GlobalOperations.js:93
> + let from = @toObject(source, "");
Why switch this given there is already null/undefined check above.
> Source/JavaScriptCore/builtins/GlobalOperations.js:118
> + let from = @toObject(source, "");
ditto
> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:62
> + let arrayLike = @toObject(items, "TypedArray.from requires an array-like
object - not null or undefined");
This seems like a semantic change given items is used below.
> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:342
> + var speciesConstructor = @toObject(constructor, ""). at speciesSymbol;
This seems like a semantic change given constructor might be null. Why change
this?
> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:383
> + var speciesConstructor = @toObject(constructor, ""). at speciesSymbol;
ditto
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1044
> + return generator.moveToDestinationIfNeeded(dst,
generator.emitToObject(generator.tempDestination(dst), src.get(), message));
Don't you want to capture tempDestination result in RefPtr?
> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2197
> + if (node->op() == ToObject)
> + clobberWorld(node->origin.semantic, clobberLimit);
Why clobberWorld?
> Source/JavaScriptCore/dfg/DFGClobberize.h:527
> + write(Heap);
Is this really true?
> Source/JavaScriptCore/dfg/DFGOperations.cpp:292
> + if (errorMessage->length()) {
> + throwVMTypeError(exec, scope, errorMessage);
> + return nullptr;
> + }
I'm really not a fan of this API. The responsibility of this node should be to
always thrown an error message in the case of undefined/null. Builtins that
don't want this shouldn't use this API.
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-4022
> - RELEASE_ASSERT(node->child1().useKind() == UntypedUse);
Why remove this? It seems like the code below is wrong if this isn't true since
you don't perform type checks
> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:445
> + if (!ident.isEmpty())
> + THROW(createTypeError(exec, ident.impl()));
Same comment. I'm really not a fan of these semantics. We should use the old
@Object if this is what we want.
More information about the webkit-reviews
mailing list