Hi Danno, thanks for the good comments thus far. The enum change is noisy
in the
CL, but it definitely improves readability!
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc#newcode109
src/code-stubs-hydrogen.cc:109: bool ensure_context,
On 2013/06/27 08:53:04, danno wrote:
Turn this boolean parameter into an enum, it makes it much easier to
understand
at the caller site what's going on.
Done.
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc#newcode110
src/code-stubs-hydrogen.cc:110: bool disable_allocation_sites,
On 2013/06/27 08:53:04, danno wrote:
Turn this boolean parameter into an enum, it makes it much easier to
understand
at the caller site what's going on. In this case, you may want to make
it part
of AllocationSite, e.g. AllocationSite::MementoState with values
AllocationSite::MEMENTOS_ENABLED and AllocationSite::MEMENTOS_DISABLED
Done.
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc#newcode114
src/code-stubs-hydrogen.cc:114: bool disable_allocation_sites,
On 2013/06/27 08:53:04, danno wrote:
Same as above
Done.
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc#newcode550
src/code-stubs-hydrogen.cc:550: return BuildArrayConstructorImpl(kind,
constructor,
On 2013/06/27 08:53:04, danno wrote:
the return clause is common code, how about factoring it out and
putting it
after the if
Thanks, I initially tried that and ran into some IfBuilder problems, but
now it works...I could eliminate BuildArrayConstructorImpl too.
https://codereview.chromium.org/17091002/diff/5001/src/code-stubs-hydrogen.cc#newcode579
src/code-stubs-hydrogen.cc:579:
On 2013/06/27 08:53:04, danno wrote:
Not sure this additional line makes a big difference :-)
Done.
https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/17091002/diff/5001/src/hydrogen.cc#newcode1784
src/hydrogen.cc:1784: // No need for a context lookup in this case.
On 2013/06/27 08:53:04, danno wrote:
Why not? What is special about smi elements?
Because the initial map on the constructor is setup for smi elements. I
added an assert in bootstrapper.cc to protect this (the assert can't
live here because this code is called before the global object is fully
set up). I also changed the code to check against
GetInitialFastElementsKind() instead of FAST_SMI_ELEMENTS.
https://codereview.chromium.org/17091002/
--
--
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.
