https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.cc#newcode855
src/hydrogen-instructions.cc:855: return new
(previous_definition->block()->zone()) HNumericConstraint(
what's previous_definition?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.cc#newcode893
src/hydrogen-instructions.cc:893: if (other == value() &&
relation.Includes(constraint(), offset, delta())) {
I don't see "Includes()" defined anywhere. Do you mean "Implies()"?
Also, more descriptive method name please. I'd like to be able to tell
from the method signature what it computes (does it check if
"this->input() relation other" holds, assuming "this->input()
this->constraint() this->value()" is true? Or the other way round?).
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.cc#newcode896
src/hydrogen-instructions.cc:896: return
input()->IsRelationTrue(relation, other, offset);
When you have two methods recursively calling each other, please put
their implementations next to each other so it's easier to see what's
going on. Also, I'm not convinced this recursive call terminates
properly in all cases.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode852
src/hydrogen-instructions.h:852: default:
default case not necessary (and not desirable)
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode857
src/hydrogen-instructions.h:857: const char* Mnemonic() const { return
MnemonicFromKind((Kind) kind_); }
no C-style casts, please. Also, no casting necessary if kind_ has the
right type in the first place.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode861
src/hydrogen-instructions.h:861: static NumericRelation None() { return
NumericRelation(NONE); }
Why do we need all of these when we have the generic constructor too?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode869
src/hydrogen-instructions.h:869: bool operator=(const NumericRelation&
other) {
I think you mean "operator==". But regardless, a .Equals() method is
generally preferred over operator overloading.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode881
src/hydrogen-instructions.h:881: case EQ: return Ne();
No.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode886
src/hydrogen-instructions.h:886: case NE: return Eq();
No.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode887
src/hydrogen-instructions.h:887: default:
no default case please
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode895
src/hydrogen-instructions.h:895: bool Implies(NumericRelation other,
int32_t offset = 0, int delta = 0) {
Why do |offset| and |delta| have different types?
Also, they should have clearer names, maybe x_delta and y_delta.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode899
src/hydrogen-instructions.h:899: (other.kind_ == EQ && offset == delta)
||
duplicate condition
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode900
src/hydrogen-instructions.h:900: (other.kind_ == GT && offset > delta)
||
No. And many of the conditions below are mixed up as well.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode908
src/hydrogen-instructions.h:908: (other.kind_ == GT && offset >= delta +
1);
Why the sudden change from "a > b" to "a >= b+1"?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode914
src/hydrogen-instructions.h:914: default:
no default case please
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode921
src/hydrogen-instructions.h:921: int8_t kind_;
Why not "Kind kind_;"?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode924
src/hydrogen-instructions.h:924: bool IsInteger32ConstantValue();
Where are these implemented?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode931
src/hydrogen-instructions.h:931: bool result = CheckRelation(relation,
other, offset) ||
"this->CheckRelation" won't compile, because |this| is HValue and
CheckRelation is defined on HNumericConstraint.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1229
src/hydrogen-instructions.h:1229: static void
AddImpliedConstraints(HInstruction* insertion_point,
I don't see an implementation for this.
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1230
src/hydrogen-instructions.h:1230: HValue* input,
I'd like a more descriptive name here. "constrained_value"?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1231
src/hydrogen-instructions.h:1231: NumericRelation constraint,
Since we have classes calld [H]NumericConstraint and NumericRelation,
please don't call a Relation constraint. (Again below.)
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1232
src/hydrogen-instructions.h:1232: HValue* value,
"value" is a bit too generic. How about "reference"? Or
"reference_point"? Or "relative_to"? (Also applies to occurrences
below.)
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1235
src/hydrogen-instructions.h:1235: static HNumericConstraint*
New(HInstruction* insertion_point,
So, we have five method with almost equal signatures and related
behavior:
HValue::InsertNumericConstraint(), HNumericConstraint::New(),
HNumericConstraints::Create(),
HNumericConstraint::AddImpliedConstraints(), and the ctor
HNumericConstraints(). Why? What's the difference? How am I supposed to
know which to use when? Can we unify them down to one or two versions?
https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#newcode1253
src/hydrogen-instructions.h:1253: //virtual void
AddInformativeDefinitions();
?
https://codereview.chromium.org/12189006/
--
--
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/groups/opt_out.
