Jakob, are you saying I don't need to check the multiplication result for
overflow? I have in the test a case where it would overflow, sending value
negative in DehoistArrayIndex before trying to add it to the existing
Array
base
offset.
I'm saying: if you fixed MaxBaseOffsetBits() to return what it's supposed to
return, then you wouldn't have to check the multiplication because it would
then
be guaranteed not to overflow. But putting all the validity checking logic
into
the new Try...() method is fine too.
LGTM with nits.
https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instructions.cc#newcode3495
src/hydrogen-instructions.cc:3495: addition_result = addition_result +
increase_by_value;
nit: consider using += for conciseness.
https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instructions.cc#newcode3497
src/hydrogen-instructions.cc:3497:
!BaseOffsetField::is_valid(base_offset + increase_by_value)) {
I'd split this check in two to make it clearer what's going on:
addition_result += increase_by_value;
if (!addition_result.IsValid()) return false;
base_offset = addition_result.ValueOrDie();
if (!BaseOffsetField::is_valid(base_offset)) return false;
bit_field_ = ...update(..., base_offset);
https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instructions.cc#newcode4090
src/hydrogen-instructions.cc:4090: addition_result = addition_result +
increase_by_value;
nit: +=
https://codereview.chromium.org/335063005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
