On 2014/10/14 11:06:00, Sven Panne wrote:
Just a few more quick remarks. I still don't understand why we suddenly
need 5
things per node (AllocateBailoutIdRange, InitializeBaseBailoutId, base_id,
leaf_base_id, kBailoutIdCount) instead of the 2 before (base_id,
num_ids), so
please don't land it yet.
Yeah I know what you mean. So before there were actually three things: you
also
had the constructors chaining the num_ids_for_subclass through to whatever
base
class allocated the base_id_. To replace chaining we have
kBailoutIdCount. To
replace the initializer in the base constructors we have
InitializeBaseBailoutId. I'd rather put AllocateBailoutIdRange in
AstNumberingVisitor but base_id_ is protected; perhaps we mark
AstNumberingVisitor as a friend class?
Then we're left with leaf_base_id(), which is weird. I'd rather replace
uses of
it with "base_id_ + super::kBailoutIdCount", for appropriate values of
super,
but that only works if base_id_ has been initialized. Instead in this
patch it
uses base_id() (which isn't base_id_ ... :/) in order to take advantage of
the
assertion there.
Dunno, if the general approach is sound then we can try to reduce the
number of
moving parts here.
https://codereview.chromium.org/636403003/diff/1/src/ast-numbering.cc
File src/ast-numbering.cc (right):
https://codereview.chromium.org/636403003/diff/1/src/ast-numbering.cc#newcode24
src/ast-numbering.cc:24: #define DEFINE_VISIT(type) \
I don't like this 2nd order macro horror, although I understand the
motivation
behind it: Separating the recursion scheme from the actual logic per node.
As it is, it obfuscates things more than it helps, so just directly
implement
all those VisitFoo directly (where Foo is an AST node type). This takes 10
minutes more to write and saves 10h reading the code later.
ACK.
https://codereview.chromium.org/636403003/diff/1/src/ast-numbering.cc#newcode250
src/ast-numbering.cc:250: bool AstNumbering::Renumber(CompilationInfo*
info) {
This is coupled too tightly: Just pass in the FunctionLiteral. It's a pity
that
we need the zone in addition (only to get the Isolate and that only for
the
stack check :-P ), but that's just the 2nd parameter then.
ACK.
https://codereview.chromium.org/636403003/diff/1/src/ast-numbering.cc#newcode256
src/ast-numbering.cc:256: if (visitor.HasStackOverflow()) return false;
return !visitor.HasStackOverflow();
ACK.
https://codereview.chromium.org/636403003/
--
--
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.
