LGTM with nits.
https://codereview.chromium.org/21505002/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/21505002/diff/3001/src/hydrogen.cc#newcode7635
src/hydrogen.cc:7635: HValue* right,
Nit: Fix indentation of arguments.
https://codereview.chromium.org/21505002/diff/3001/src/hydrogen.cc#newcode7731
src/hydrogen.cc:7731: HValue* context) {
Hmmm, having 8 arguments is not very nice, but let's see how it will be
used elsewhere and perhaps refactor that. A TODO would be nice.
https://codereview.chromium.org/21505002/diff/3001/src/hydrogen.cc#newcode7736
src/hydrogen.cc:7736: if (op != Token::ADD ||
Brain explosion caused by heavy use of negation... o_O Pushing the
negation outwards, things are much clearer:
if (!(op == Token::ADD &&
(left_type->Maybe(Type::String()) ||
right_type->Maybe(Type::String())))) {
Naming the expression would be even better:
bool maybe_string_addition = ...
if (!maybe_string_addition) {
This would even make the comment in the if body obsolete.
"Comments are a sign of our inability to express things in code." :-)
https://codereview.chromium.org/21505002/
--
--
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.
