Regarding the unit tests: Unless there is a deep reason (which I currently
can't
see), I would prefer having all tests in a single file. We usually group
related
tests together like this.
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc#newcode1901
src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" :
"_ncti");
Nit: This probably fits into the previous line.
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
Please print the full instruction here, not just the variable name,
otherwise the output is not very helpful. Same for instr below. (We did
this previously BTW).
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr)
{
I think this helper method is actually less clear than inlining it at
its call sites and simplifying the resulting code.
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
We call this phase twice, so it would be good to name these differently,
making the visualizer output more readable. Note that HPhase doesn't
keep a copy of the string, just a pointer.
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
As Jakob already pointed out, I don't think we need this, see below.
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
src/hydrogen.cc:5421: // Mark initial root instructions for dead code
elimination.
Replace this comment and the ones below by extracted methods with
explanatory names (just proposals below):
MarkRoots(&worklist);
MarkLive(&worklist);
Sweep();
http://www.refactoring.com/catalog/extractMethod.html ;-)
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
src/hydrogen.cc:5456: while (!philist.is_empty()) {
Just fold this into the previous loop, no need for a separate list.
https://codereview.chromium.org/14676011/
--
--
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.
