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

Issue 12223071: ES6 symbols: Introduce Symbol class, along with abstract Name class (Closed)

Created:
7 years, 10 months ago by rossberg
Modified:
7 years, 9 months ago
Reviewers:
Michael Starzinger
CC:
v8-dev, danno
Visibility:
Public.

Description

ES6 symbols: Introduce Symbol class, along with abstract Name class The new instance type 'Symbol' represents ES6 symbols (a.k.a. private/unique names). Currently, symbols are simple data objects that only carry a hash code, random-generated upon allocation. The new type 'Name' now serves as the common super class for strings and symbols, and is supposed to represent property names. We will eventually migrate APIs from String to Name for the standard key type. Strings and symbols share the same hash field representation, via the Name class. This way, we should be able to use the same code paths for symbols and internalized strings in most cases. Also, Symbol's instance type code is allocated adjacent to internalized string codes in the enum, allowing a simple range check for the common case. Baseline CL: https://codereview.chromium.org/12210083/ R=mstarzinger@chromium.org BUG= Committed: http://code.google.com/p/v8/source/detail?r=13783

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -89 lines) Patch
M include/v8.h View 2 chunks +4 lines, -4 lines 0 comments Download
M src/factory.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/factory.cc View 1 chunk +8 lines, -0 lines 0 comments Download
M src/heap.h View 2 chunks +8 lines, -0 lines 0 comments Download
M src/heap.cc View 1 2 chunks +33 lines, -0 lines 0 comments Download
M src/objects.h View 12 chunks +130 lines, -78 lines 0 comments Download
M src/objects.cc View 1 2 chunks +4 lines, -0 lines 0 comments Download
M src/objects-debug.cc View 2 chunks +10 lines, -0 lines 0 comments Download
M src/objects-inl.h View 4 chunks +15 lines, -7 lines 0 comments Download
M src/objects-printer.cc View 1 3 chunks +10 lines, -0 lines 0 comments Download
M src/objects-visiting.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M test/cctest/SConscript View 1 chunk +1 line, -0 lines 0 comments Download
M test/cctest/cctest.gyp View 1 chunk +1 line, -0 lines 0 comments Download
A test/cctest/test-symbols.cc View 1 1 chunk +63 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
rossberg
7 years, 10 months ago (2013-02-11 17:52:28 UTC) #1
Michael Starzinger
LGTM, just nits. https://codereview.chromium.org/12223071/diff/1/src/heap.cc File src/heap.cc (right): https://codereview.chromium.org/12223071/diff/1/src/heap.cc#newcode5180 src/heap.cc:5180: STATIC_ASSERT(HeapNumber::kSize <= Page::kNonCodeObjectAreaSize); Both comment and ...
7 years, 10 months ago (2013-02-12 10:03:53 UTC) #2
rossberg
7 years, 10 months ago (2013-02-13 10:04:28 UTC) #3
https://codereview.chromium.org/12223071/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/12223071/diff/1/src/heap.cc#newcode5180
src/heap.cc:5180: STATIC_ASSERT(HeapNumber::kSize <=
Page::kNonCodeObjectAreaSize);
On 2013/02/12 10:03:53, Michael Starzinger wrote:
> Both comment and assertion are out-dated and use the wrong size constant.

Oops, done.

https://codereview.chromium.org/12223071/diff/1/test/cctest/test-symbols.cc
File test/cctest/test-symbols.cc (right):

https://codereview.chromium.org/12223071/diff/1/test/cctest/test-symbols.cc#n...
test/cctest/test-symbols.cc:11: #include "cctest.h"
On 2013/02/12 10:03:53, Michael Starzinger wrote:
> Can we alpha-sort the includes?

Done. Also removed obsolete import of "zone-inl.h".

Powered by Google App Engine
This is Rietveld 408576698