(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.h#newcode66
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.h#newcode78
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.h#newcode149
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#newcode2110
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#newcode2123
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
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.
