LMBTMN (looks much better to me now; that implies LGTM). Another round of
nits.
https://codereview.chromium.org/22876009/diff/71027/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/22876009/diff/71027/src/flag-definitions.h#newcode297
src/flag-definitions.h:297: DEFINE_bool(unreachable_code_elimination,
true,
nit: Looks like it fits into one line now.
https://codereview.chromium.org/22876009/diff/71027/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/22876009/diff/71027/src/hydrogen-instructions.cc#newcode2889
src/hydrogen-instructions.cc:2889:
HConstant::cast(left())->DataEquals(HConstant::cast(right()));
suggestion: We could use Equals() instead of DataEquals() here.
Admittedly that performs some redundant checks, but would allow us to
keep HConstant::DataEquals() protected and not break abstraction. I am
fine either way, your choice.
https://codereview.chromium.org/22876009/diff/71027/src/hydrogen-mark-unreachable.h
File src/hydrogen-mark-unreachable.h (right):
https://codereview.chromium.org/22876009/diff/71027/src/hydrogen-mark-unreachable.h#newcode40
src/hydrogen-mark-unreachable.h:40: : HPhase("H_Propagate unrechable
mark", graph) { }
nit: s/H_Propagate unrechable mark/H_Mark unreachable/
https://codereview.chromium.org/22876009/diff/71027/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):
https://codereview.chromium.org/22876009/diff/71027/src/ia32/lithium-codegen-ia32.h#newcode35
src/ia32/lithium-codegen-ia32.h:35: #include "lithium-codegen.h"
nit: Can we alpha-sort the includes.
https://codereview.chromium.org/22876009/diff/71027/src/lithium-codegen.cc
File src/lithium-codegen.cc (right):
https://codereview.chromium.org/22876009/diff/71027/src/lithium-codegen.cc#newcode144
src/lithium-codegen.cc:144: #undef __
nit: Looks obsolete, let's drop it.
https://codereview.chromium.org/22876009/diff/71027/src/lithium-codegen.h
File src/lithium-codegen.h (right):
https://codereview.chromium.org/22876009/diff/71027/src/lithium-codegen.h#newcode41
src/lithium-codegen.h:41: class LCodeGenBase V8_FINAL BASE_EMBEDDED {
This V8_FINAL looks wrong because we actually inherit from this class.
Doesn't actually speak for the usefulness off these macros in the first
place. :)
https://codereview.chromium.org/22876009/diff/71027/src/lithium-codegen.h#newcode94
src/lithium-codegen.h:94: #undef __
nit: Looks obsolete, let's drop it.
https://codereview.chromium.org/22876009/
--
--
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.
