LGTM. I like it. Only nits.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs-hydrogen.cc#newcode857
src/code-stubs-hydrogen.cc:857: HValue* res = NULL;
nit: s/res/result/
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc
File src/code-stubs.cc (right):
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode213
src/code-stubs.cc:213: op_name,
nit: Should fit into one line.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode296
src/code-stubs.cc:296: for (int i = 0; i < 5; i++) {
nit: s/5/ARRAY_SIZE(arithop)/
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode313
src/code-stubs.cc:313: for (int i = 0; i < 5; i++) {
nit: s/5/ARRAY_SIZE(bitop)/
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode377
src/code-stubs.cc:377: UNREACHABLE();
nit: Drop the default case, the switch should be exhaustive here.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode483
src/code-stubs.cc:483: t = handle(Type::Union(t, handle(Type::Double(),
isolate)), isolate);
Missing break statement.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.cc#newcode485
src/code-stubs.cc:485: t = handle(Type::Union(t,
handle(Type::Signed32(), isolate)), isolate);
Missing break statement.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.h#newcode1081
src/code-stubs.h:1081: OverwriteMode mode() { return mode_; }
nit: These three can be made const.
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.h#newcode1105
src/code-stubs.h:1105: void init();
nit: s/init/Initialize/
https://codereview.chromium.org/24072013/diff/29001/src/code-stubs.h#newcode1113
src/code-stubs.h:1113: class ResultStateField: public
BitField<State, 9, 3> {};
nit: Move the ResultStateField down to after the RightBoolField, I think
that would be easier to read.
https://codereview.chromium.org/24072013/diff/29001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/24072013/diff/29001/src/hydrogen-instructions.h#newcode766
src/hydrogen-instructions.h:766: if (other->CheckFlag(f)) SetFlag(f);
nit: Indentation is off.
https://codereview.chromium.org/24072013/diff/29001/src/isolate.cc
File src/isolate.cc (right):
https://codereview.chromium.org/24072013/diff/29001/src/isolate.cc#newcode2343
src/isolate.cc:2343: binopStub.InitializeInterfaceDescriptor(
Let's move this into a static BinaryOpStub::InitializeForIsolate()
helper.
https://codereview.chromium.org/24072013/diff/29001/src/type-info.cc
File src/type-info.cc (right):
https://codereview.chromium.org/24072013/diff/29001/src/type-info.cc#newcode407
src/type-info.cc:407: return;
Terminal return is obsolete.
https://codereview.chromium.org/24072013/diff/29001/src/type-info.cc#newcode425
src/type-info.cc:425: Handle<Type> unknown = handle(Type::None(),
isolate_);
nit: Can we use the constructor syntax instead of an assignment for new
handles?
Handle<Type> unknown(Type::None(), isolate_);
https://codereview.chromium.org/24072013/
--
--
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.
