WIP: Parser: delay string internalization.

WIP: Parser: delay string internalization.

This is so that the Parser can be ran on a background thread.

BUG=

[rossberg, 2014-05-08 12:52:07]

Like it so far. My main suggestions for improvement are a cleaner interface for
AST Literal, and renaming ParserSymbol(Table) to something that does neither
mention the parser, nor "symbol" -- see below.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode852
src/ast.h:852: ForStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
Nit: line length

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode894
src/ast.h:894: ForEachStatement(Zone* zone,
ZoneList<ParserSymbolTable::Symbol*>* labels, int pos)
Nit: line length

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode934
src/ast.h:934: ForInStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
Nit: line length

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode997
src/ast.h:997: ForOfStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
Nit: line length

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1157
src/ast.h:1157: SwitchStatement(Zone* zone,
ZoneList<ParserSymbolTable::Symbol*>* labels, int pos)
Nit: line length

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1382
src/ast.h:1382: if (!value_.is_null()) {
Is it an invariant that at least one of value_, string_, strings_ is non-null?
If so, might be worth asserting it here. Or alternatively, add an "else
UNREACHABLE" below.

Likewise, one could assert that at most one of string_ and strings_ can't be
non-null.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1402
src/ast.h:1402: ParserSymbolTable::Symbol* string() const { return string_; }
Maybe call these symbol/symbol_?

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1442
src/ast.h:1442: ZoneList<ParserSymbolTable::Symbol*>* strings_;
symbols_? Also, add a comment that this is to represent arrays of strings.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode2383
src/ast.h:2383: if (raw_inferred_name_ != NULL) {
I'm surprised that inferred_name_ does not take precedence.

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table.h
File src/parser-symbol-table.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:66: class ParserSymbolTable {
How about naming this just SymbolTable? It's not quite specific to the parser,
as symbols are used after parsing, and in modules that don't (or shouldn't) know
anything about parsing (e.g., scopes, interfaces).

Also, I feel tempted to suggest using a different term than "symbol", in order
to avoid confusion with ES6 symbols. One common synonym is "atom".

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:78: void AlwaysInternalize(Isolate* isolate) {
How does this differ from Internalize (which also sets isolate_)?

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:149: // FIXME: these are easily created with a macro.
I was about to suggest that. :)

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1031
src/parser.cc:1031: // Start using the heap (before scope goes out of scope).
"scope is exited"?

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1093
src/parser.cc:1093: literal->string() != NULL) {
Would be nicer to have an IsString predicate on Literal, so that you don't have
to rely on such internals here. The string accessor should assert IsString
instead of returning NULL.

Maybe it would be even better to have disjoint subclasses StringLiteral and
StringArrayLiteral?

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1815
src/parser.cc:1815: if (!var->name().is_null()) {
Why is this case needed?

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h#newcode...
src/preparser.h:2110: this->ReportMessageAt(location, "strict_eval_arguments",
(const char*)NULL,
Nit: style guide forbids C casts

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h#newcode...
src/preparser.h:2123: this->ReportMessageAt(location, message, (const
char*)NULL, true);
Same here

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h
File src/scanner.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h#newcode381
src/scanner.h:381: ParserSymbol* CurrentString(ParserSymbolTable* symbol_table);
CurrentSymbol?

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h#newcode382
src/scanner.h:382: ParserSymbol* NextString(ParserSymbolTable* symbol_table);
NextSymbol?

https://codereview.chromium.org/231073002/diff/880002/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/231073002/diff/880002/src/scopes.cc#newcode89
src/scopes.cc:89: : NULL),
Hey, you uglified it! :)

https://codereview.chromium.org/231073002/diff/880002/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/scopes.h#newcode76
src/scopes.h:76: enum HeapMode {
Where is this used?

[ulan, 2014-05-21 15:01:21]

https://codereview.chromium.org/231073002/diff/920001/src/parser-symbol-table.cc
File src/parser-symbol-table.cc (right):

https://codereview.chromium.org/231073002/diff/920001/src/parser-symbol-table...
src/parser-symbol-table.cc:57: if (string1->hash != string2->hash) return false;
I think HashMap guarantees that the hashes are equal upon calling this function.

https://codereview.chromium.org/231073002/diff/920001/src/parser-symbol-table...
src/parser-symbol-table.cc:72: {
How about second order macro for this? :)

[marja, 2014-05-21 17:16:44]

https://codereview.chromium.org/231073002/diff/920001/src/parser-symbol-table.cc
File src/parser-symbol-table.cc (right):

https://codereview.chromium.org/231073002/diff/920001/src/parser-symbol-table...
src/parser-symbol-table.cc:57: if (string1->hash != string2->hash) return false;
On 2014/05/21 15:01:22, ulan wrote:
> I think HashMap guarantees that the hashes are equal upon calling this
function.

Actually, not. Turns out this case is sometimes hit. (We might bucket elements
with different hash functions together. Which is good. We don't want to have a
2^32 element hash array or anything like that.)

[marja, 2014-05-21 17:22:48]

Err, s/hash functions/hash values/.

[marja, 2014-06-03 08:48:19]

(Marking comments done here. I'll create a new CL as this is moving away from
the WIP state...)

https://codereview.chromium.org/231073002/diff/880002/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode852
src/ast.h:852: ForStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode894
src/ast.h:894: ForEachStatement(Zone* zone,
ZoneList<ParserSymbolTable::Symbol*>* labels, int pos)
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode934
src/ast.h:934: ForInStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode997
src/ast.h:997: ForOfStatement(Zone* zone, ZoneList<ParserSymbolTable::Symbol*>*
labels, int pos)
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1157
src/ast.h:1157: SwitchStatement(Zone* zone,
ZoneList<ParserSymbolTable::Symbol*>* labels, int pos)
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1382
src/ast.h:1382: if (!value_.is_null()) {
On 2014/05/08 12:52:08, rossberg wrote:
> Is it an invariant that at least one of value_, string_, strings_ is non-null?
> If so, might be worth asserting it here. Or alternatively, add an "else
> UNREACHABLE" below.
> 
> Likewise, one could assert that at most one of string_ and strings_ can't be
> non-null.

This is now refactored so that we have another Literal class which takes care of
this indirection (literal can be bool / double / int / string / array of
strings) -> obsolete.

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1402
src/ast.h:1402: ParserSymbolTable::Symbol* string() const { return string_; }
On 2014/05/08 12:52:08, rossberg wrote:
> Maybe call these symbol/symbol_?

This is also in the other class now, and I think it's reasonable to call it
"string" there (as in "string / boolean / number").

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode1442
src/ast.h:1442: ZoneList<ParserSymbolTable::Symbol*>* strings_;
On 2014/05/08 12:52:08, rossberg wrote:
> symbols_? Also, add a comment that this is to represent arrays of strings.

Ditto (added comment about this to the literal class)

https://codereview.chromium.org/231073002/diff/880002/src/ast.h#newcode2383
src/ast.h:2383: if (raw_inferred_name_ != NULL) {
On 2014/05/08 12:52:08, rossberg wrote:
> I'm surprised that inferred_name_ does not take precedence.

Doesn't matter, because only one of them will be set. Made that explicit in the
code with ASSERTs and nullifying the raw_inferred_name_ if name_ is set, and
vice versa.

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table.h
File src/parser-symbol-table.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:66: class ParserSymbolTable {
On 2014/05/08 12:52:08, rossberg wrote:
> How about naming this just SymbolTable? It's not quite specific to the parser,
> as symbols are used after parsing, and in modules that don't (or shouldn't)
know
> anything about parsing (e.g., scopes, interfaces).
> 
> Also, I feel tempted to suggest using a different term than "symbol", in order
> to avoid confusion with ES6 symbols. One common synonym is "atom".

Done (AstStringTable etc. as discussed offline).

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:78: void AlwaysInternalize(Isolate* isolate) {
On 2014/05/08 12:52:08, rossberg wrote:
> How does this differ from Internalize (which also sets isolate_)?

-> Removed AlwaysInternalize

https://codereview.chromium.org/231073002/diff/880002/src/parser-symbol-table...
src/parser-symbol-table.h:149: // FIXME: these are easily created with a macro.
On 2014/05/08 12:52:08, rossberg wrote:
> I was about to suggest that. :)

Done.

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1031
src/parser.cc:1031: // Start using the heap (before scope goes out of scope).
On 2014/05/08 12:52:08, rossberg wrote:
> "scope is exited"?

Actually, this is not needed any more, so moved the internalizing later.

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1093
src/parser.cc:1093: literal->string() != NULL) {
On 2014/05/08 12:52:08, rossberg wrote:
> Would be nicer to have an IsString predicate on Literal, so that you don't
have
> to rely on such internals here. The string accessor should assert IsString
> instead of returning NULL.
> 
> Maybe it would be even better to have disjoint subclasses StringLiteral and
> StringArrayLiteral?


The StringLiteral / StringArrayLiteral / Literal split didn't work out (as
discussed offline).

There's now a IsString() func in the other Literal class.

https://codereview.chromium.org/231073002/diff/880002/src/parser.cc#newcode1815
src/parser.cc:1815: if (!var->name().is_null()) {
On 2014/05/08 12:52:08, rossberg wrote:
> Why is this case needed?

Done

(It wasn't.)

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h#newcode...
src/preparser.h:2110: this->ReportMessageAt(location, "strict_eval_arguments",
(const char*)NULL,
On 2014/05/08 12:52:08, rossberg wrote:
> Nit: style guide forbids C casts

This change is no longer needed

https://codereview.chromium.org/231073002/diff/880002/src/preparser.h#newcode...
src/preparser.h:2123: this->ReportMessageAt(location, message, (const
char*)NULL, true);
On 2014/05/08 12:52:08, rossberg wrote:
> Same here

This change is no longer needed

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h
File src/scanner.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h#newcode381
src/scanner.h:381: ParserSymbol* CurrentString(ParserSymbolTable* symbol_table);
On 2014/05/08 12:52:08, rossberg wrote:
> CurrentSymbol?

Done.

https://codereview.chromium.org/231073002/diff/880002/src/scanner.h#newcode382
src/scanner.h:382: ParserSymbol* NextString(ParserSymbolTable* symbol_table);
On 2014/05/08 12:52:08, rossberg wrote:
> NextSymbol?

Done.

https://codereview.chromium.org/231073002/diff/880002/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/231073002/diff/880002/src/scopes.cc#newcode89
src/scopes.cc:89: : NULL),
On 2014/05/08 12:52:08, rossberg wrote:
> Hey, you uglified it! :)

Done.

https://codereview.chromium.org/231073002/diff/880002/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/231073002/diff/880002/src/scopes.h#newcode76
src/scopes.h:76: enum HeapMode {
On 2014/05/08 12:52:08, rossberg wrote:
> Where is this used?

Nowhere :) -> removed

[marja, 2014-06-03 08:50:35]

.... and closing this one; the new CL is

https://codereview.chromium.org/314603004