Almost there. Mostly style comments left:
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2617 src/hydrogen.cc:2617: *offset = is_sub ? - constant->Integer32Value() I'd break this expression like this: *offset = is_sub ? -constant->Integer32Value() : constant->Integer32Value(); http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2628 src/hydrogen.cc:2628: BoundsCheckKey(HValue* index_base, HValue* length) { This is may be rewritten shorter using an initializer list: BoundsCheckKey(HValue* index_base, HValue* length) : index_base_(index_base), length_(length) { } http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2763 src/hydrogen.cc:2763: original_value, Don't the arguments fit on one line? http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: HConstant** constant) { Fits on the previous line? http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2778 src/hydrogen.cc:2778: (*constant)->Unlink(); We use normally the helper DeleteAndReplaceWith(NULL) for deleting instructions from the graph. It has some additional assertions that the value does not have any uses left. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2807 src/hydrogen.cc:2807: } Fits on the previous line. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2835 src/hydrogen.cc:2835: bb_data_list = new(bb->zone()) BoundsCheckBbData(key, Since HGraph already has a zone() accessor, you can just write: new (zone()) http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2850 src/hydrogen.cc:2850: int32_t new_lower_offset = offset < data->LowerOffset() ? offset Maybe the expressions here better readable as: int32_t new_lower_offset = offset < data->LowerOffset() ? offset : data->LowerOffset(); http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884 src/hydrogen.cc:2884: AssertNoAllocation no_gc; Why would you want to assert no allocation here? It does not hurt, but does not seem necessary. We use this assert in places where we use raw pointers to the JS heap instead of handles (e.g. many function in objects.cc, runtime.cc). http://codereview.chromium.org/10032029/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
