Looks mostly good. My only concern is with the name AstStringTable and its
construction API for AstValues.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc
File src/ast-string-table.cc (right):
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode71
src/ast-string-table.cc:71: if (!is_one_byte_) return false;
Merge into next if?
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode145
src/ast-string-table.cc:145: default:
Don't use a default, so that you can get compiler warnings.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode149
src/ast-string-table.cc:149: return false;
Have the UNREACHABLE here.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode198
src/ast-string-table.cc:198: default:
Same here.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h
File src/ast-string-table.h (right):
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode28
src/ast-string-table.h:28: #ifndef V8_AST_STRING_TABLE_H_
How about renaming this module to AstValue, since strings are just one
facet now.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode56
src/ast-string-table.h:56: // The string is not NULL-terminated, use
length() to find out the length.
Nit: null-terminated, since it's not NULL the pointer.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode92
src/ast-string-table.h:92: enum Type {
Can we add the missing New* constructors below and make this enum
private?
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode175
src/ast-string-table.h:175: explicit AstValue(Type t) :
...and get rid of this one.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode250
src/ast-string-table.h:250: const AstValue* NewValue(const AstString*
string);
It's weird that these functions are all members of AstStringTable.
Rename this to AstValueFactory or s.th like that?
Also, I'd prefer to name all these NewSomething consistently, for
Something in String, Number, Boolean, Undefined, etc. The Value suffix
seems redundant once these functions are in a proper place.
https://codereview.chromium.org/314603004/diff/1/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode669
src/ast.h:669: int pos)
Nit: this should fit on prev line
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode2347
src/ast.h:2347: inferred_name_ = Handle<String>();
Hm, can't you ASSERT that this is already the case?
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3028
src/ast.h:3028: const AstString* name, int pos) {
Nit: fits prev line
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3156
src/ast.h:3156: Literal* NewLiteral(const AstString* string, int pos) {
I think we should name all these `NewSomethingLiteral`.
https://codereview.chromium.org/314603004/diff/1/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/314603004/diff/1/src/parser.h#newcode522
src/parser.h:522: const char* char_arg = NULL,
Nit: char_arg -> arg
https://codereview.chromium.org/314603004/diff/1/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/314603004/diff/1/src/preparser.h#newcode152
src/preparser.h:152: typename Traits::Type::Zone* extra_param = NULL,
extra_param??
https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h
File src/prettyprinter.h (right):
https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h#newcode49
src/prettyprinter.h:49: void PrintLiteral(Handle<Object> value, bool
quote);
Maybe unify these two into one PrintValue(AstValue*)?
https://codereview.chromium.org/314603004/diff/1/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/314603004/diff/1/src/scopes.cc#newcode43
src/scopes.cc:43: Entry* p =
ZoneHashMap::Lookup(const_cast<AstString*>(name), name->hash(),
Would it be possible to avoid this ugly casting by fixing the type of
Lookup itself?
https://codereview.chromium.org/314603004/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
