Patch 8 addresses the last round of comments.
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()
On 2012/04/25 09:40:48, fschneider wrote:
I'd break this expression like this:
*offset = is_sub ? -constant->Integer32Value()
: constant->Integer32Value();
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2628
src/hydrogen.cc:2628: BoundsCheckKey(HValue* index_base, HValue* length)
{
BoundsCheckKey(HValue* index_base, HValue* length)
: index_base_(index_base),
length_(length) { }
Done but with the colon be on the 1st line: is it ok anyway?
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2763
src/hydrogen.cc:2763: original_value,
On 2012/04/25 09:40:48, fschneider wrote:
Don't the arguments fit on one line?
No: 81 chars wide :-(
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2775
src/hydrogen.cc:2775: HConstant** constant) {
On 2012/04/25 09:40:48, fschneider wrote:
Fits on the previous line?
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2778
src/hydrogen.cc:2778: (*constant)->Unlink();
On 2012/04/25 09:40:48, fschneider wrote:
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.
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2807
src/hydrogen.cc:2807: }
On 2012/04/25 09:40:48, fschneider wrote:
Fits on the previous line.
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2835
src/hydrogen.cc:2835: bb_data_list = new(bb->zone())
BoundsCheckBbData(key,
On 2012/04/25 09:40:48, fschneider wrote:
Since HGraph already has a zone() accessor, you can just write:
new (zone())
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2850
src/hydrogen.cc:2850: int32_t new_lower_offset = offset <
data->LowerOffset() ? offset
On 2012/04/25 09:40:48, fschneider wrote:
Maybe the expressions here better readable as:
int32_t new_lower_offset = offset < data->LowerOffset()
? offset
: data->LowerOffset();
Done.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884
src/hydrogen.cc:2884: AssertNoAllocation no_gc;
On 2012/04/25 09:40:48, fschneider wrote:
Why would you want to assert no allocation here? It does not hurt, but
does not
seem necessary.
It is necessary: I was hitting asserts without it.
This happened computing key hashes calling HConstant::Hashcode().
http://codereview.chromium.org/10032029/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
