LGTM
http://codereview.chromium.org/113258/diff/1/2 File src/jump-target.cc (right): http://codereview.chromium.org/113258/diff/1/2#newcode118 Line 118: if (left->is_register() && right->is_register() && This function looks like a lot of duplicated code. The for conditions are separate, even though they do almost, or exactly, the same. It's not easy to see that they are all merely checking whether the two FrameElements are compatible, and if they are, pick the unsynced one, if any. When the bodies were simpler, it might not have been a problem, but I do think it is now. It seems like it could be simplified (of a sort) by combining the tree conditions that all have the same body. Something like: if (... || ... || ...) { FrameElement result = left->is_synced ? right : left; result->set_static_type(static_type); return result; } Perhaps make some utility function for the test, or for parts of the it. The one at this comment looks like it could be named IsSameRegister. You can even add the first test as well, since it doesn't seem to matter which element is used. http://codereview.chromium.org/113258/diff/1/2#newcode182 Line 182: FrameElement element = initial_frame->elements_[i]; If we look this element up again later, would it perhaps be easier to store a pointer instead of making a copy? http://codereview.chromium.org/113258/diff/1/2#newcode208 Line 208: elements[i] = Combine(element, &reaching_frames_[j]->elements_[i]); Why do we read the element on each round of the inner loop, instead of reading it before entering the loop, updating only the variable during the loop, and then setting it back after the loop? http://codereview.chromium.org/113258 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
