LGTM, but you may want to get Florian's opinion too.
http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode2063 src/hydrogen.cc:2063: HValue* true_value = invert_true() Add a helper for this pattern. GetConstantBoolean(true|false). http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3227 src/hydrogen.cc:3227: { Consider factoring the inner scope out into a separate named helper function. http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3527 src/hydrogen.cc:3527: { Helper? http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3795 src/hydrogen.cc:3795: HSubgraph* subgraph = CreateBranchSubgraph(environment()); Helper? Maybe the three helpers can actually share something - not sure. http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode4239 src/hydrogen.cc:4239: if (subgraph()->HasExit()) { This code is duplicated below (along with a nice big comment). I would refactor - maybe call it DoInline. http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h File src/hydrogen.h (right): http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode563 src/hydrogen.h:563: // functions for expressions. I guess you can only 'fill' a context once. Would it make sense to verify that this is the way it is used by having a bool is_filled_ in debug mode? http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode580 src/hydrogen.h:580: int original_count_; The comment mentions stack height. The name mentions count. Maybe somehow link the two? http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode672 src/hydrogen.h:672: HEnvironment* environment() const { return subgraph()->environment(); } Long term maybe this should be Environment? http://codereview.chromium.org/5620007/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
