Next round of comments...
http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2592 src/hydrogen.cc:2592: if (check->index()->IsAdd()) { If we extract a function like 'MatchOffsetExpresssion(HValue** index_base, HConstant** constant)' and massage CoverCheck a tiny bit, it should be straightforward to handle the HMinus case, too. I leave it up to you to do this now or in a separate CL. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2637 src/hydrogen.cc:2637: // class BoundsCheckBbData { Remove that line. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2646 src/hydrogen.cc:2646: static void RemoveCheck(HBoundsCheck* check) { I think this method should be generalized and moved to HTemplateInstruction (under a better name). In general, static methods are a code smell, with very few exceptions like factory methods. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2659 src/hydrogen.cc:2659: if (check_simulate_ != NULL) { Use a guard clause instead, see http://martinfowler.com/refactoring/catalog/replaceNestedConditionalWithGuardClauses.html http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2689 src/hydrogen.cc:2689: if (check->next()->IsSimulate()) { Using a ternary operator here and using initializer syntax instead is more "C++"-like. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2720 src/hydrogen.cc:2720: intptr_t hash = key->Hash(); Wrong type, but this is nicer when inlined, anyway. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2726 src/hydrogen.cc:2726: intptr_t hash = key->Hash(); Inline again. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2727 src/hydrogen.cc:2727: ZoneHashMap::Entry* entry = Lookup(key, hash, true); No need to name this explicitly. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2749 src/hydrogen.cc:2749: if (i->IsBoundsCheck()) { 'if (!i->IsBoundsCheck()) continue;' reduces nesting a bit and is clearer IMHO. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2769 src/hydrogen.cc:2769: BoundsCheckBbData::RemoveCheck(check); This will probably be something like 'check->Remove()' or whatever the function will be called. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2773 src/hydrogen.cc:2773: isolate()->counters()->array_bounds_checks_removed()->Increment(); I don't think that the increment here is correct, because the check is removed conditionally. http://codereview.chromium.org/10032029/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
