Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 546803003: Update ObjectToString to Harmony-draft algorithm (Closed)

Created:
4 years, 10 months ago by caitp (gmail)
Modified:
4 years, 9 months ago
CC:
v8-dev
Project:
v8
Visibility:
Public.

Description

Update ObjectToString to Harmony-draft algorithm Updates Object.prototype.toString() to use algorithm described in harmony drafts. Currently, the behaviour is essentially the same as ES262's version, however this changes when internal structures such as Promise make use of symbolToStringTag (as they are supposed to, see v8:3241), and changes further once Symbol.toStringTag is exposed publicly. BUG=v8:3241, v8:3502 LOG=N R=dslomov@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=24783

Patch Set 1 #

Patch Set 2 : Add @@toStringTag to some objects + tests #

Total comments: 8

Patch Set 3 : Put ES6 behaviour behind flag, fixup tests, refactor kBuiltinToStringTags #

Total comments: 9

Patch Set 4 : Make ObjectProtoToString() work like ES6 Object.prototype.toString-ish #

Total comments: 5

Patch Set 5 : Use intern'd strings instead of creating vectors #

Total comments: 9

Patch Set 6 : Add more test cases for test-api/ObjectProtoToString #

Patch Set 7 : Add lots more tests, make it safer #

Total comments: 13

Patch Set 8 : Nits, few more tests #

Total comments: 3

Patch Set 9 : Test nits #

Patch Set 10 : Teach NoSideEffectToString() that ObjectToString was extended #

Total comments: 4

Patch Set 11 : Rebase, rename ObjectToString #

Unified diffs Side-by-side diffs Delta from patch set Stats (+456 lines, -17 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M include/v8.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M src/api.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +58 lines, -0 lines 0 comments Download
M src/array.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M src/arraybuffer.js View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +4 lines, -0 lines 0 comments Download
M src/collection.js View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M src/generator.js View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
A src/harmony-tostring.js View 1 2 3 4 5 6 7 8 9 1 chunk +66 lines, -0 lines 0 comments Download
M src/heap/heap.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -1 line 0 comments Download
M src/messages.js View 1 2 3 4 5 6 7 8 9 10 4 chunks +7 lines, -5 lines 0 comments Download
M src/promise.js View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/symbol.js View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M src/v8natives.js View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -2 lines 0 comments Download
M src/weak-collection.js View 1 2 chunks +4 lines, -0 lines 0 comments Download
M test/cctest/test-api.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +124 lines, -0 lines 0 comments Download
M test/mjsunit/es6/collections.js View 1 2 3 chunks +11 lines, -2 lines 0 comments Download
M test/mjsunit/es6/generators-objects.js View 1 2 2 chunks +2 lines, -1 line 0 comments Download
A test/mjsunit/es6/object-tostring.js View 1 2 3 4 5 6 7 1 chunk +133 lines, -0 lines 0 comments Download
M test/mjsunit/es6/promises.js View 1 2 3 4 5 6 1 chunk +11 lines, -3 lines 0 comments Download
M test/mjsunit/es6/symbols.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M test/mjsunit/harmony/typedarrays.js View 1 2 3 4 5 6 7 8 9 10 3 chunks +7 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 41 (4 generated)
caitp (gmail)
Alternative version of https://codereview.chromium.org/227413004/ which uses the proper Symbol implementation (although Symbol.toStringTag is still not ...
4 years, 10 months ago (2014-09-05 15:36:29 UTC) #1
caitp (gmail)
Adopt updated algorithm for Object.prototype.toString, which makes use of Symbol.toStringTag --- expose symbolToStringTag via Symbol.toStringTag ...
4 years, 9 months ago (2014-10-10 04:34:02 UTC) #3
caitp (gmail)
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217 src/v8natives.js:217: var kBuiltinStringTags = { ObjectCreate(null) would be better, I ...
4 years, 9 months ago (2014-10-10 14:56:28 UTC) #4
rossberg
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217 src/v8natives.js:217: var kBuiltinStringTags = { On 2014/10/10 14:56:28, caitp wrote: ...
4 years, 9 months ago (2014-10-17 11:19:31 UTC) #6
caitp (gmail)
On 2014/10/17 11:19:31, rossberg wrote: > https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js > File src/v8natives.js (right): > > https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode217 > ...
4 years, 9 months ago (2014-10-17 12:31:44 UTC) #7
arv (Not doing code reviews)
https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/546803003/diff/70001/src/v8natives.js#newcode241 src/v8natives.js:241: return kBuiltinStringTags[tag]; Maybe make the kBuiltinStringTags a "set" and ...
4 years, 9 months ago (2014-10-17 14:40:51 UTC) #9
caitp (gmail)
Moved to `harmony-tostring.js` loaded behind a flag, use null prototype for kBuiltinStringTags, fixed tests and ...
4 years, 9 months ago (2014-10-17 18:05:57 UTC) #10
arv (Not doing code reviews)
There is also ObjectProtoToString in C++. We should update that to match ES6 as well.
4 years, 9 months ago (2014-10-17 18:22:26 UTC) #11
arv (Not doing code reviews)
On 2014/10/17 18:22:26, arv wrote: > There is also ObjectProtoToString in C++. We should update ...
4 years, 9 months ago (2014-10-17 18:22:53 UTC) #12
Dmitry Lomov (no reviews)
Some comments https://codereview.chromium.org/546803003/diff/240001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/546803003/diff/240001/src/flag-definitions.h#newcode165 src/flag-definitions.h:165: DEFINE_BOOL(harmony_tostring, false, "enable ES6 version of Object.prototype.toString") ...
4 years, 9 months ago (2014-10-17 19:29:06 UTC) #14
caitp (gmail)
On 2014/10/17 18:22:53, arv wrote: > On 2014/10/17 18:22:26, arv wrote: > > There is ...
4 years, 9 months ago (2014-10-17 20:12:13 UTC) #15
caitp (gmail)
Added the ObjectProtoToString stuff. https://codereview.chromium.org/546803003/diff/260001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/546803003/diff/260001/src/api.cc#newcode3412 src/api.cc:3412: return v8::String::NewFromUtf8(isolate, "[object ~Arguments]"); I'm ...
4 years, 9 months ago (2014-10-17 22:00:49 UTC) #16
Dmitry Lomov (no reviews)
Some more comments. https://codereview.chromium.org/546803003/diff/240001/src/symbol.js File src/symbol.js (right): https://codereview.chromium.org/546803003/diff/240001/src/symbol.js#newcode115 src/symbol.js:115: $Symbol.prototype, symbolToStringTag, "Symbol", DONT_ENUM | READ_ONLY); ...
4 years, 9 months ago (2014-10-18 08:41:49 UTC) #17
Dmitry Lomov (no reviews)
Also, like Erik said, feel free to make a separate CL for ObjectProtoToString.
4 years, 9 months ago (2014-10-18 11:45:34 UTC) #18
caitp (gmail)
https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc#newcode13505 test/cctest/test-api.cc:13505: CHECK(value->IsString() && value->Equals(v8_str("[object ~Array]"))); On 2014/10/18 08:41:49, Dmitry Lomov ...
4 years, 9 months ago (2014-10-18 13:34:35 UTC) #19
Dmitry Lomov (no reviews)
On 2014/10/18 13:34:35, caitp wrote: > https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc > File test/cctest/test-api.cc (right): > > https://codereview.chromium.org/546803003/diff/260001/test/cctest/test-api.cc#newcode13505 > ...
4 years, 9 months ago (2014-10-18 14:40:21 UTC) #20
Dmitry Lomov (no reviews)
Getting there! Now I reviewed the whole thing, sorry for giving out comments a bit ...
4 years, 9 months ago (2014-10-18 15:16:48 UTC) #21
Dmitry Lomov (no reviews)
One more process-y thing: even though this is not a large feature, it has its ...
4 years, 9 months ago (2014-10-18 15:43:19 UTC) #22
caitp (gmail)
On 2014/10/18 15:43:19, Dmitry Lomov (chromium) wrote: > One more process-y thing: even though this ...
4 years, 9 months ago (2014-10-18 20:23:36 UTC) #23
Dmitry Lomov (no reviews)
Almost there, only nits and just two (actually three) more tests. https://codereview.chromium.org/546803003/diff/320001/src/api.cc File src/api.cc (right): ...
4 years, 9 months ago (2014-10-18 21:24:19 UTC) #24
Dmitry Lomov (no reviews)
https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object-tostring.js File test/mjsunit/es6/object-tostring.js (right): https://codereview.chromium.org/546803003/diff/320001/test/mjsunit/es6/object-tostring.js#newcode20 test/mjsunit/es6/object-tostring.js:20: Error: [ Error, TypeError, RangeError, SyntaxError, ReferenceError, EvalError, URIError ...
4 years, 9 months ago (2014-10-18 21:27:15 UTC) #25
caitp (gmail)
https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc#newcode13537 test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate); On 2014/10/18 21:24:19, Dmitry Lomov ...
4 years, 9 months ago (2014-10-18 21:27:40 UTC) #26
caitp (gmail)
https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc#newcode13537 test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate); Er, I mean `Symbol.toStringTag` is ...
4 years, 9 months ago (2014-10-18 21:35:01 UTC) #27
Dmitry Lomov (no reviews)
https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc#newcode13537 test/cctest/test-api.cc:13537: Local<v8::Object> obj = v8::Object::New(isolate); On 2014/10/18 21:27:40, caitp wrote: ...
4 years, 9 months ago (2014-10-18 21:45:00 UTC) #28
caitp (gmail)
On 2014/10/18 21:45:00, Dmitry Lomov (chromium) wrote: > https://codereview.chromium.org/546803003/diff/320001/test/cctest/test-api.cc > File test/cctest/test-api.cc (right): > > ...
4 years, 9 months ago (2014-10-19 00:27:43 UTC) #29
Dmitry Lomov (no reviews)
LGTM with two more small nits. https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/546803003/diff/340001/test/cctest/test-api.cc#newcode13513 test/cctest/test-api.cc:13513: CcTest::InitializeVM(); Ah, ok, ...
4 years, 9 months ago (2014-10-19 08:49:52 UTC) #30
Dmitry Lomov (no reviews)
Looks like you have not run make quickcheck on this patch; if you do, test/webkit/fast/js/Promise-resolve-with-itself.js ...
4 years, 9 months ago (2014-10-20 19:12:35 UTC) #31
caitp (gmail)
On 2014/10/20 19:12:35, Dmitry Lomov (chromium) wrote: > Looks like you have not run > ...
4 years, 9 months ago (2014-10-20 19:25:44 UTC) #32
caitp (gmail)
On 2014/10/20 19:25:44, caitp wrote: > On 2014/10/20 19:12:35, Dmitry Lomov (chromium) wrote: > > ...
4 years, 9 months ago (2014-10-21 01:21:02 UTC) #33
caitp (gmail)
yeah, with change all passing (except for a few intl issues that always fail for ...
4 years, 9 months ago (2014-10-21 01:29:28 UTC) #34
Dmitry Lomov (no reviews)
Comments - we should make things safe. Also, please rebase this patch (you will need ...
4 years, 9 months ago (2014-10-21 06:11:53 UTC) #35
caitp (gmail)
Updated to use the new flag/experimentals machinery https://codereview.chromium.org/546803003/diff/380001/src/messages.js File src/messages.js (right): https://codereview.chromium.org/546803003/diff/380001/src/messages.js#newcode226 src/messages.js:226: if (IS_OBJECT(obj) ...
4 years, 9 months ago (2014-10-21 14:54:45 UTC) #36
Dmitry Lomov (no reviews)
lgtm, will land
4 years, 9 months ago (2014-10-21 15:35:45 UTC) #37
Dmitry Lomov (no reviews)
Committed patchset #11 (id:400001) manually as 24783 (presubmit successful).
4 years, 9 months ago (2014-10-21 17:22:01 UTC) #38
rossberg
On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote: > Committed patchset #11 (id:400001) manually as 24783 ...
4 years, 9 months ago (2014-10-22 06:41:57 UTC) #39
Dmitry Lomov (no reviews)
On 2014/10/22 06:41:57, rossberg wrote: > On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote: > > ...
4 years, 9 months ago (2014-10-22 07:59:21 UTC) #40
caitp (gmail)
4 years, 9 months ago (2014-10-22 12:16:15 UTC) #41
Message was sent while issue was closed.
On 2014/10/22 07:59:21, Dmitry Lomov (chromium) wrote:
> On 2014/10/22 06:41:57, rossberg wrote:
> > On 2014/10/21 17:22:01, Dmitry Lomov (chromium) wrote:
> > > Committed patchset #11 (id:400001) manually as 24783 (presubmit
successful).
> > 
> > Did you run test262-es6? This seems to break it (S15.2.4.2_A6 failing).
> 
> I know I didn't. Caitlin caitp@, will you be able to look into this soon-ish?
> Thanks!

Looks like a trivial fix, I'll submit a patch for it. I didn't know the ES5
tests were running for the test262-es6 run

Powered by Google App Engine
This is Rietveld 408576698