https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#newcode379
src/hydrogen-instructions.cc:379: while (next != NULL) {
I don't like endless loops very much.
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#newcode410
src/hydrogen-instructions.cc:410: return
dominated->CheckFlag(kIDefsProcessingDone) || dominated->IsPhi();
I think this condition is the wrong way round: it's dominated if it has
*not* been visited yet and is *not* a Phi.
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode873
src/hydrogen-instructions.h:873: static bool
CheckDominanceFollowingDefinitionList(
I'm not too happy with this method's name. "Check" sounds like some
assertion. Also, it would be nice to model this after
HBasicBlock::Dominates(). Lastly, I'm not sure what
"FollowingDefinitionList" tries to tell me.
So how about "DominatesByBlockWalking"? And "DominatesByProcessedFlag"
below? Or just "DominatesSlow"/"DominatesFast"?
Ideally, I'd make it a non-static method too (so you can call it as
"dominator->DominatesSlow(other)", but that doesn't fit with passing
method pointers (see below). For templatized use, it would work if the
template parameter was a bool or enum used in an if/else (which the
compiler would optimize away).
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode874
src/hydrogen-instructions.h:874: HValue* dominator, HValue* dominated);
nit: for method/function declarations (contrary to calls), put each
argument onto its own line unless everything (including the method name)
fits onto one line. In this case:
static bool CheckDominanceFollowingDefinitionList(HValue* dominator,
HValue* dominated);
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode876
src/hydrogen-instructions.h:876: // When we are setting up informaative
definitions we use a flag to mark
nit: s/informaative/informative/
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode880
src/hydrogen-instructions.h:880: HValue* dominator, HValue* dominated);
same here
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode884
src/hydrogen-instructions.h:884: template<bool
(*CheckDominance)(HValue*, HValue*)>
Please use a typedef for the function signature definition. (Put it
right before the first declaration of a function of this type; even
though syntactically they don't use it, they do semantically "implement
the interface" defined by the typedef.)
Is there any particular reason you templatized this method, as opposed
to just passing in a function pointer?
https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#newcode889
src/hydrogen-instructions.h:889: while (!uses.Done()) {
I'm used to seeing for-loops for iterating over uses. Let's be
consistent with that pattern:
for (HUseIterator uses = input->uses(); !uses.Done(); uses.Advance()) {
... }
https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc#newcode3592
src/hydrogen.cc:3592: EliminateRedundantBoundsChecks();
nit: {} please
https://codereview.chromium.org/12079042/
--
--
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.
