First round of comments.
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.cc#newcode3918
src/hydrogen-instructions.cc:3918: if (use_rep.IsNone() ||
use_rep.IsSmi()) continue;
'if (!use_rep.IsNone() && !use_rep.IsSmi()) return true;' is a bit more
readable, and a predicate on Representation with a good name would be
even better (but I fail to come up with a good one :-).
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#newcode3454
src/hydrogen-instructions.h:3454: if (right()->IsConstant()) return
false;
Let's hope that this change doesn't have subtle performance
implications...
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#newcode3488
src/hydrogen-instructions.h:3488: if (!FLAG_smi_binop) {
Nit: I think that
if (new_rep.IsSmi() && !FLAG_smi_binop) ...
is more readable.
Using a ternary instead of an assignment to a parameter would be nice,
too, but this is a personal preference.
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#newcode4637
src/hydrogen-instructions.h:4637: SetFlag(kTruncatingToSmi);
I don't understand this part. Why is this correct? At least a comment is
needed.
https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#newcode5984
src/hydrogen-instructions.h:5984: if (elements_kind <=
EXTERNAL_SHORT_ELEMENTS) {
Stuff like this has been there before, but it is ugly as hell: This
can't be understood without seeing the ElementsKind definition (and this
even lacks a comment that the order of enums carries some semantics...
:-P
https://codereview.chromium.org/20070005/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/20070005/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode670
src/ia32/lithium-codegen-ia32.cc:670: return
reinterpret_cast<int32_t>(Smi::FromInt(value));
Evil! ;-)
https://codereview.chromium.org/20070005/
--
--
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.
