On 2015/08/22 01:19:13, bradn wrote:
https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):
https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-visitor.cc#newcode63
src/ast-expression-visitor.cc:63: RECURSE(Visit(stmt));
So Visit has baked in a short-circuit:
#define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS() \
public: \
void Visit(AstNode* node) final { \
if (!CheckStackOverflow()) node->Accept(this); \
} \
So in terms of the suggestion you made by chat, wrapping this in
VisitStmt or
somesuch, won't reduce the depth. Also, the macro has the advantage that
in a
loop like this (or anywhere else where more than one visit call happens in
sequence), that you immediately exit the current function, instead of
trying
each and backing out after each guard. Putting it in a function prevents
the
return from skipping the rest of the function.
I could do for RECURSE what I did in this version for
VisitDeeperExpression,
return a bool indicating if a short-circuit return should happen, but I'm
not
sure that's more readable than the macro.
WDYT?
I'll try to catch you at some point to talk it over.
After seeing the alternatives I think I like RECURSE and RECURSE_EXPRESSION
macros the best.
I do wonder if in general there might be some more coherent way to handle
the
stack checks. It seems a shame to need to semi-reliably put in what
amounts to
non-zero cost exception handling.
I would be interested in chatting about what the constraints really are
here
in
terms of using page faults etc.
In terms of specifically the AST visitor, would it be worthwhile to
restructure
it to use a non-c-stack recursive pattern? I.e. Visit doesn't actually
call
Accept directly, but instead pushes pending Visits onto a heap stack in
the
visitor and an outer iterative loop dispatches the visits in the same
order
they
currently happen?
That's beyond the scope of this CL. We've gotten by with other visitors
remaining recursive, so I don't see the need. Besides, you end up needing a
stack of both AST nodes and an index of some kind to keep track of where you
were in the recursion, and that gets ugly quick.
https://codereview.chromium.org/1288773007/
--
--
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.
