[webkit-reviews] review granted: [Bug 178726] [JSC] Introduce @toObject : [Attachment 324690] Patch

[bugzilla-daemon at webkit.org]

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.