Suggestions for iDef infrastructure classes.

Suggestions for iDef infrastructure classes.

[Jakob Kummerow, 2013-02-04 17:06:09]

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#...
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#...
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#...
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#n...
src/hydrogen-instructions.h:852: default:
default case not necessary (and not desirable)

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
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#n...
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#n...
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#n...
src/hydrogen-instructions.h:881: case EQ: return Ne();
No.

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:886: case NE: return Eq();
No.

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:887: default:
no default case please

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
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#n...
src/hydrogen-instructions.h:899: (other.kind_ == EQ && offset == delta) ||
duplicate condition

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
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#n...
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#n...
src/hydrogen-instructions.h:914: default:
no default case please

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:921: int8_t kind_;
Why not "Kind kind_;"?

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:924: bool IsInteger32ConstantValue();
Where are these implemented?

https://codereview.chromium.org/12189006/diff/1/src/hydrogen-instructions.h#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
src/hydrogen-instructions.h:1253: //virtual void AddInformativeDefinitions();
?