LGTM, just a couple of nits. Sorry for being so nitty.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode115
src/code-stubs-hydrogen.cc:115: void InstallCode(HValue* js_function,
HValue* native_context,
nit: Can we prefix these methods with either "Build" or "Emit" to make
it clear that they don't actually "do what they say" but "emit code that
does what they say"?
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode916
src/code-stubs-hydrogen.cc:916: void
CodeStubGraphBuilderBase::InstallCode(HValue* js_function,
suggestion: How about calling this method "EmitInstallOptimizedCode" and
the one below "EmitInstallCode", that would make their relationship
clearer.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode929
src/code-stubs-hydrogen.cc:929: // No need for write barrier as
JSFunction object is in the new space.
nit: The second sentence of the comment is obsolete. Crankshaft is smart
enough to figure that out on it's own, no need to worry about WBs here.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode955
src/code-stubs-hydrogen.cc:955: void
CodeStubGraphBuilderBase::InstallOptimizedClosure(
suggestion: This method could be called "EmitSearchOptimizedCodeMap" or
"EmitInstallFromOptimizedCodeMap", that better describes what it does.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode1055
src/code-stubs-hydrogen.cc:1055: HInstruction* undefined_value =
Add<HConstant>(factory->undefined_value());
Use graph()->GetConstantUndefined() to get to the "undefined" constant.
This also spares you the trouble of passing it around everywhere.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode1074
src/code-stubs-hydrogen.cc:1074: // Initialize the rest of the function.
We don't have to update the
nit: The second sentence of the comment is obsolete. Crankshaft is smart
enough to figure that out on it's own, no need to worry about WBs here.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode1082
src/code-stubs-hydrogen.cc:1082: HInstruction* the_hole =
Add<HConstant>(factory->the_hole_value());
Use graph()->GetConstantHole() here instead.
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.cc#newcode3867
src/hydrogen-instructions.cc:3867: // Context::SlotOffset(index)
Instead of the comment, lets add an assert that the result of your
computation is actually correct ...
ASSERT_EQ(offset, Context::SlotOffset(index) + kHeapObjectTag);
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h#newcode5165
src/hydrogen-instructions.h:5165: HValue* js_function() { return
OperandAt(0); }
nit: s/js_function/function/ ... or ... s/js_function/closure/
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h#newcode5169
src/hydrogen-instructions.h:5169: private:
nit: Empty new-line before the "private:" header.
https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/22562002/diff/15001/src/runtime.cc#newcode7961
src/runtime.cc:7961: // directly to properties.
nit: Comment does not apply, lets just drop it.
https://codereview.chromium.org/22562002/diff/15001/src/runtime.h
File src/runtime.h (right):
https://codereview.chromium.org/22562002/diff/15001/src/runtime.h#newcode398
src/runtime.h:398: F(NewClosureBailout, 1, 1) \
The other runtime functions jumped to by a stub bailout are post-fixed
with a "FromStubFailure" name, I think we should stick to that
convention (or alternatively rename the others as well).
https://codereview.chromium.org/22562002/
--
--
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.
