Nits addressed, thanks they were good ones, esp. those that addressed my
rather
muddled function names!
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,
On 2013/08/27 08:12:51, Michael Starzinger wrote:
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"?
Done.
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,
On 2013/08/27 08:12:51, Michael Starzinger wrote:
suggestion: How about calling this method "EmitInstallOptimizedCode"
and the one
below "EmitInstallCode", that would make their relationship clearer.
Good idea. Though I use "Build" instead of "Emit," because
CodeStubGraphBuilderBase already has a collection of "Build" helpers for
array functions; it seems more consistent with this file.
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.
On 2013/08/27 08:12:51, Michael Starzinger wrote:
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.
Done.
https://codereview.chromium.org/22562002/diff/15001/src/code-stubs-hydrogen.cc#newcode955
src/code-stubs-hydrogen.cc:955: void
CodeStubGraphBuilderBase::InstallOptimizedClosure(
On 2013/08/27 08:12:51, Michael Starzinger wrote:
suggestion: This method could be called "EmitSearchOptimizedCodeMap"
or
"EmitInstallFromOptimizedCodeMap", that better describes what it does.
Done.
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());
On 2013/08/27 08:12:51, Michael Starzinger wrote:
Use graph()->GetConstantUndefined() to get to the "undefined"
constant. This
also spares you the trouble of passing it around everywhere.
Done.
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
On 2013/08/27 08:12:51, Michael Starzinger wrote:
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.
Done.
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());
On 2013/08/27 08:12:51, Michael Starzinger wrote:
Use graph()->GetConstantHole() here instead.
Done.
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)
On 2013/08/27 08:12:51, Michael Starzinger wrote:
Instead of the comment, lets add an assert that the result of your
computation
is actually correct ...
ASSERT_EQ(offset, Context::SlotOffset(index) + kHeapObjectTag);
Done.
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); }
On 2013/08/27 08:12:51, Michael Starzinger wrote:
nit: s/js_function/function/ ... or ... s/js_function/closure/
Done.
https://codereview.chromium.org/22562002/diff/15001/src/hydrogen-instructions.h#newcode5169
src/hydrogen-instructions.h:5169: private:
On 2013/08/27 08:12:51, Michael Starzinger wrote:
nit: Empty new-line before the "private:" header.
Done.
https://codereview.chromium.org/22562002/diff/15001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
https://codereview.chromium.org/22562002/diff/15001/src/ia32/lithium-ia32.h#newcode1788
src/ia32/lithium-ia32.h:1788: LOperand* js_function() { return
inputs_[0]; }
I continued the 's/js_function/function' work into here.
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.
On 2013/08/27 08:12:51, Michael Starzinger wrote:
nit: Comment does not apply, lets just drop it.
Done.
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.
