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.h#newcode66
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.h#newcode78
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.h#newcode149
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#newcode2110
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#newcode2123
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?
https://codereview.chromium.org/231073002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
