Thanks for comments! rossberg@, could you take another look?
The first new patch set is just rebasing, no interesting changes.
The second new patch set contains the code review modifications.
I also detected a bug with the FuncNameInferrer tests I had just added an
fixed
it as a separate patch set.
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;
On 2014/06/11 14:58:12, rossberg wrote:
Merge into next if?
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode145
src/ast-string-table.cc:145: default:
On 2014/06/11 14:58:12, rossberg wrote:
Don't use a default, so that you can get compiler warnings.
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode149
src/ast-string-table.cc:149: return false;
On 2014/06/11 14:58:12, rossberg wrote:
Have the UNREACHABLE here.
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newcode198
src/ast-string-table.cc:198: default:
On 2014/06/11 14:58:12, rossberg wrote:
Same here.
Done (except now I cannot have "UNREACHABLE()" at the bottom, because
the cases don't return).
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_
On 2014/06/11 14:58:12, rossberg wrote:
How about renaming this module to AstValue, since strings are just one
facet
now.
Done (as discussed, renamed AstStringTable to AstValueFactory, and it
takes care of both AstStrings and AstValues).
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.
On 2014/06/11 14:58:12, rossberg wrote:
Nit: null-terminated, since it's not NULL the pointer.
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode92
src/ast-string-table.h:92: enum Type {
On 2014/06/11 14:58:12, rossberg wrote:
Can we add the missing New* constructors below and make this enum
private?
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newcode175
src/ast-string-table.h:175: explicit AstValue(Type t) :
On 2014/06/11 14:58:12, rossberg wrote:
...and get rid of this one.
No, I still need this one to be able to construct them. It'll be only
used by AstValueFactory::NewUndefined() etc., so not visible to outside
(Parser or such).
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);
On 2014/06/11 14:58:12, rossberg wrote:
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.
Done.
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)
On 2014/06/11 14:58:12, rossberg wrote:
Nit: this should fit on prev line
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode2347
src/ast.h:2347: inferred_name_ = Handle<String>();
On 2014/06/11 14:58:12, rossberg wrote:
Hm, can't you ASSERT that this is already the case?
Done (note that inferred_name() above already ASSERTed that only one of
the names is something meaningful.
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3028
src/ast.h:3028: const AstString* name, int pos) {
On 2014/06/11 14:58:12, rossberg wrote:
Nit: fits prev line
Done.
https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3156
src/ast.h:3156: Literal* NewLiteral(const AstString* string, int pos) {
On 2014/06/11 14:58:12, rossberg wrote:
I think we should name all these `NewSomethingLiteral`.
Done. And I named the one which takes an AstValue* "NewSpecialLiteral".
(It's used for null, undefined and the hole.)
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,
On 2014/06/11 14:58:12, rossberg wrote:
Nit: char_arg -> arg
Done.
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,
On 2014/06/11 14:58:12, rossberg wrote:
extra_param??
Oops, this was unintentional. Fixed. It's actually called unconsistently
"extra_param" and "zone" in other places, but I don't want to touch that
in this CL...
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);
On 2014/06/11 14:58:13, rossberg wrote:
Maybe unify these two into one PrintValue(AstValue*)?
I can't, since not all the strings to be printed are inside AstValues.
For example node->proxy()->name() is an AstString*, a label is an
AstString*.
What I could do is to change the one that takes Handle<Object> value to
take an AstValue* and dispatch based on its type, but that's a bit
unnecessary change.
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(),
On 2014/06/11 14:58:13, rossberg wrote:
Would it be possible to avoid this ugly casting by fixing the type of
Lookup
itself?
I tried it, but it's a bit messy. I'd rather not do it on this CL. I
added a FIXME here to do a follow-up.
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.
