Great work, I like it. Some comments, though.
https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h
File src/ast-value-factory.h (right):
https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h#newcode281
src/ast-value-factory.h:281: const AstString* GetOneByteString(const
Vector<const uint8_t>& literal);
Hm, isn't the idea of the Vector abstraction that this shouldn't be
necessary?
https://codereview.chromium.org/335293004/diff/130035/src/ast-value-factory.h
File src/ast-value-factory.h (right):
https://codereview.chromium.org/335293004/diff/130035/src/ast-value-factory.h#newcode60
src/ast-value-factory.h:60: // Creates a cons string. The only allowed
operations for cons strings are
Disallowing a whole set of operations in some cases is kind of nasty, as
is always using space for the cross product of both possible
representations.
Do you think it would be possible to have the two different forms of
strings as concrete subclasses of (an abstract) AstString (say
AstConsString and AstRawString?). Then you can get rid of the type
thingy and a bunch of associated asserts, potentially use virtual
dispatch for the common functions, and checked downcasts to get to the
representation-specific ones.
https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsing.cc#newcode801
test/cctest/test-parsing.cc:801: i::AstValueFactory
ast_value_factory(&zone, 0);
Why is a hash seed of 0 okay here? Maybe add a comment.
https://codereview.chromium.org/335293004/
--
--
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.
