https://codereview.chromium.org/14990014/diff/3001/src/ast.cc
File src/ast.cc (right):
https://codereview.chromium.org/14990014/diff/3001/src/ast.cc#newcode421
src/ast.cc:421: for_in_type_ =
static_cast<ForInType>(oracle->ForInType(this));
Hmmm, where should types like ForInType live in the long run? I don't
have a good idea yet, casts are :-P, circular dependencies are even more
:-P
https://codereview.chromium.org/14990014/diff/3001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode376
src/ast.h:376: byte to_boolean_types() const { return to_boolean_types_;
}
Do we still have problems with #include cycles, so we have to use "byte"
here? If not, the real type would be nicer.
https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode896
src/ast.h:896: ForInType for_in_type_;
Can we keep the fields and methods in a common order, e.g. syntactical
parts (i.e. the "real" AST children), then type annotations, and finally
bailout/feedback IDs? I don't care too much about the actual order, more
about the consistency. Separating the parts by empty lines or comments
might be nice, too.
https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc#newcode9783
src/hydrogen.cc:9783: ASSERT(feedback->IsSmi());
Looks a bit redundant now...
https://codereview.chromium.org/14990014/diff/3001/src/typing.cc
File src/typing.cc (right):
https://codereview.chromium.org/14990014/diff/3001/src/typing.cc#newcode84
src/typing.cc:84: CHECK_ALIVE(VisitStatements(stmt->statements()));
Hmmm, do we really need a CHECK_ALIVE here? I thought we need this only
for early return. Anyway, can't we somehow have this centrally in
AstVisitor::Visit? There is already a check, but it is not 100% clear to
me why that isn't enough. Maybe monad by hand?
https://codereview.chromium.org/14990014/diff/3001/src/typing.h
File src/typing.h (right):
https://codereview.chromium.org/14990014/diff/3001/src/typing.h#newcode48
src/typing.h:48: void* operator new(size_t size, Zone* zone) {
What is the reason for the new/delete definitions here? If they are
really needed, a comment is appropriate. Furthermore, this is
effectively making AstTyper a ZoneObject (why?). Is it just avoiding
multiple inheritance?
*sigh* In general, we should really templatize things more, this would
just get a ZoneAllocationPolicy then. We are extremely ad-hoc in such
things. :-(
https://codereview.chromium.org/14990014/
--
--
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/groups/opt_out.
