Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issue 1306303003: [es6] Implement spec compliant ToPrimitive in the runtime. (Closed)

Created:
5 years, 3 months ago by Benedikt Meurer
Modified:
5 years, 3 months ago
CC:
v8-dev, Yang
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] Implement spec compliant ToPrimitive in the runtime. This is the first step towards a spec compliant ToPrimitive implementation (and therefore spec compliant ToNumber, ToString, ToName, and friends). It adds support for the @@toPrimitive symbol that was introduced with ES2015, and also adds the new Symbol.prototype[@@toPrimitive] and Date.prototype[@@toPrimitive] initial properties. There are now runtime functions for %ToPrimitive, %ToNumber and %ToString, which do the right thing and should be used as fallbacks instead of the hairy runtime.js implementations. I will do the same for the other conversion operations mentioned by the spec in follow up CLs. Once everything is in place we can look into optimizing things further, so that we don't always call into the runtime. Also fixed Date.prototype.toJSON to be spec compliant. R=mstarzinger@chromium.org, yangguo@chromium.org BUG=v8:4307 LOG=y Committed: https://crrev.com/f6c6d713b4a1bb94457571f9e627ce2dea531b68 Cr-Commit-Position: refs/heads/master@{#30434}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address Michis comments. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+665 lines, -148 lines) Patch
M src/accessors.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/api.cc View 5 chunks +6 lines, -6 lines 0 comments Download
M src/arm/code-stubs-arm.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/arm64/code-stubs-arm64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/compiler/linkage.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M src/contexts.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/date.js View 3 chunks +20 lines, -1 line 0 comments Download
M src/debug/debug.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/execution.h View 2 chunks +0 lines, -8 lines 0 comments Download
M src/execution.cc View 3 chunks +2 lines, -14 lines 0 comments Download
M src/full-codegen/arm/full-codegen-arm.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/arm64/full-codegen-arm64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/ia32/full-codegen-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/mips/full-codegen-mips.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/mips64/full-codegen-mips64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/ppc/full-codegen-ppc.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/x64/full-codegen-x64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/full-codegen/x87/full-codegen-x87.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/heap/heap.h View 3 chunks +4 lines, -1 line 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/json-stringifier.h View 1 chunk +2 lines, -2 lines 0 comments Download
M src/macros.py View 1 chunk +3 lines, -0 lines 0 comments Download
M src/messages.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/messages.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/mips/code-stubs-mips.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/mips64/code-stubs-mips64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/objects.h View 6 chunks +38 lines, -3 lines 0 comments Download
M src/objects.cc View 1 7 chunks +297 lines, -10 lines 1 comment Download
M src/objects-inl.h View 2 chunks +16 lines, -0 lines 0 comments Download
M src/ppc/code-stubs-ppc.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/runtime.js View 3 chunks +0 lines, -3 lines 0 comments Download
M src/runtime/runtime.h View 1 chunk +5 lines, -0 lines 0 comments Download
M src/runtime/runtime-i18n.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/runtime/runtime-numbers.cc View 1 chunk +2 lines, -72 lines 0 comments Download
M src/runtime/runtime-object.cc View 1 chunk +56 lines, -0 lines 0 comments Download
M src/runtime/runtime-scopes.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/symbol.js View 4 chunks +16 lines, -0 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/x87/code-stubs-x87.cc View 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-heap.cc View 2 chunks +3 lines, -5 lines 0 comments Download
A test/mjsunit/harmony/to-number.js View 1 chunk +57 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/to-primitive.js View 1 chunk +110 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 14 (4 generated)
Benedikt Meurer
5 years, 3 months ago (2015-08-28 08:23:22 UTC) #1
Benedikt Meurer
Yang: Please do overall review. Michi: Please review compiler changes. Michael: Please look at code ...
5 years, 3 months ago (2015-08-28 08:24:29 UTC) #3
mvstanton
LGTM code stubs.
5 years, 3 months ago (2015-08-28 08:34:36 UTC) #4
Michael Starzinger
LGTM modulo spec changes, didn't verify those. https://codereview.chromium.org/1306303003/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1306303003/diff/1/src/objects.cc#newcode6017 src/objects.cc:6017: Handle<Object> exoticToPrim; ...
5 years, 3 months ago (2015-08-28 08:52:13 UTC) #5
Benedikt Meurer
https://codereview.chromium.org/1306303003/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1306303003/diff/1/src/objects.cc#newcode6017 src/objects.cc:6017: Handle<Object> exoticToPrim; On 2015/08/28 08:52:13, Michael Starzinger wrote: > ...
5 years, 3 months ago (2015-08-28 08:54:53 UTC) #6
Yang
lgtm.
5 years, 3 months ago (2015-08-28 09:00:34 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306303003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306303003/20001
5 years, 3 months ago (2015-08-28 09:00:55 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 3 months ago (2015-08-28 09:21:30 UTC) #11
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/f6c6d713b4a1bb94457571f9e627ce2dea531b68 Cr-Commit-Position: refs/heads/master@{#30434}
5 years, 3 months ago (2015-08-28 09:21:55 UTC) #12
adamk
5 years, 3 months ago (2015-08-28 19:36:15 UTC) #14
Message was sent while issue was closed.
I'd appreciate if someone from the Language team was included on changes like
this. There's some behavior change in this patch that may cause problems on the
web, see below.

https://codereview.chromium.org/1306303003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1306303003/diff/20001/src/objects.cc#newcode6020
src/objects.cc:6020: GetMethod(receiver,
isolate->factory()->to_primitive_symbol()), Object);
This is going to change behavior when invoking ToPrimitive on access-checked
objects. Code which would have previously worked will now throw. See
https://code.google.com/p/v8/issues/detail?id=4289 for a similar issue with
@@isConcatSpreadable.

I have a test case demonstrating the problem, will attach it to the bug.

Powered by Google App Engine
This is Rietveld 408576698