https://codereview.chromium.org/636403003/diff/270001/src/ast-numbering.h
File src/ast-numbering.h (right):
https://codereview.chromium.org/636403003/diff/270001/src/ast-numbering.h#newcode13
src/ast-numbering.h:13: class AstNumbering {
This is not really a class, using a namespace might be more honest.
https://codereview.chromium.org/636403003/diff/270001/src/ast-numbering.h#newcode15
src/ast-numbering.h:15: // Assign type feedback IDs, bailout IDs, and
other state to an AST node
Remove the "and other state" part for now.
https://codereview.chromium.org/636403003/diff/270001/src/ast-numbering.h#newcode19
src/ast-numbering.h:19: // failure.
Actually, this is not true: We just failed to set the IDs, nothing else
should have been damaged.
https://codereview.chromium.org/636403003/diff/270001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/636403003/diff/270001/src/ast.h#newcode379
src/ast.h:379: base_id_(INT_MIN) {}
Hmmm, INT_MIN is a bit ugly, I think just using -1 (or
BailoutId::None().ToInt()) here and adding
static_asssert<kNoneId < 0, "kNoneId must be negative">
to the TypeFeedbackId and BailoutId classes is a bit better. Forcing
TypeFeedbackId and BailoutId to have a common superclass containing
None() etc. doesn't sound right, either.
Another option is simply not doing the initialization at all (and remove
the DCHECK below) and rely on our tooling to find uninitialized access
(ASAN will do this).
https://codereview.chromium.org/636403003/diff/270001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/636403003/diff/270001/src/compiler.cc#newcode644
src/compiler.cc:644: if (!AstNumbering::Renumber(info->function(),
info->zone())) return false;
Not a fault of this CL, but we should *really, really* try to unify all
those compilation pipeline fragments, so that there is a single place
for this. But this should not be done in this CL...
https://codereview.chromium.org/636403003/diff/270001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/636403003/diff/270001/src/parser.cc#newcode882
src/parser.cc:882: zone(), ast_value_factory());
A nice little cleanup which could be done on the way: Remove the Zone*
argument from the AstNodeFactory, the zone can (and should!) be
retrieved fromt the AstValueFactory (add a getter there). It simply
doesn't make sense to pretend that AST nodes and AST values can live in
different zones.
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.
