Thanks for comments!
One additional fix: I made AstString ctors private, since they're only
meant to
be used by AstValueFactory (which is already a friend).
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);
On 2014/06/23 09:44:27, rossberg wrote:
Hm, isn't the idea of the Vector abstraction that this shouldn't be
necessary?
It's not a huge difference; this will copy the length and the data
pointer (not the actual data); I just got a comment in another code
review that I should use const Vector&; and why not... that's copying
one pointer instead of 2.
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
On 2014/06/23 09:44:27, rossberg wrote:
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.
Done.
I named the classes AstStringBase, AstString (the same as AstString used
to be before) and AstConsString.
Most places use AstStrings (like before), only function names are stored
as AstStringBase. We don't need to downcast ever. The only needed
operations for cons strings are string() and length(), and AstStringBase
takes care of them.
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);
On 2014/06/23 09:44:27, rossberg wrote:
Why is a hash seed of 0 okay here? Maybe add a comment.
Why wouldn't it be? Afaics, it's usually 0, except when
FLAG_randomize_hashes is there.
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.
