All changes made, except the cleanup of EmitNamedPropertyLoad in the full
code
generator.
http://codereview.chromium.org/573009/diff/15/29
File src/full-codegen.cc (right):
http://codereview.chromium.org/573009/diff/15/29#newcode1055
src/full-codegen.cc:1055: VisitForValue(prop->obj(), kAccumulator);
On 2010/02/05 09:02:33, Kevin Millikin wrote:
This needs a comment that you are copying the temporary (the object)
with one
copy allocated to the accumulator and one to the stack.
Done.
http://codereview.chromium.org/573009/diff/15/29#newcode1078
src/full-codegen.cc:1078: EmitNamedPropertyLoad(prop);
On 2010/02/05 09:02:33, Kevin Millikin wrote:
As a simple cleanup in another change, could you make
EmitNamedPropertyLoad/EmitKeyedPropertyLoad use the expression context
to
determine what to do with their result?
Will do later.
http://codereview.chromium.org/573009/diff/15/18
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/573009/diff/15/18#newcode6593
src/ia32/codegen-ia32.cc:6593: if (persist_after_get_) {
On 2010/02/05 09:02:33, Kevin Millikin wrote:
Since this code and the corresponding !persist_after_get_ bracket the
load in
both arms of the if, I think it's more obvious to have it outside the
if.
Done.
http://codereview.chromium.org/573009/diff/15/17
File src/ia32/full-codegen-ia32.cc (right):
http://codereview.chromium.org/573009/diff/15/17#newcode1567
src/ia32/full-codegen-ia32.cc:1567: VisitForValue(prop->obj(),
kAccumulator);
On 2010/02/05 09:02:33, Kevin Millikin wrote:
Add a simple comment that you are copying the value, one copy in the
accumulator
and one on the stack.
Done.
http://codereview.chromium.org/573009/diff/15/23
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/573009/diff/15/23#newcode1231
src/ia32/ic-ia32.cc:1231: void LoadIC::Generate(MacroAssembler* masm,
const ExternalReference& f) {
On 2010/02/05 09:02:33, Kevin Millikin wrote:
As with the store ICs, I think you can rename Generate ==>
GenerateMiss and
remove the extra argument.
Done.
http://codereview.chromium.org/573009/diff/15/19
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/573009/diff/15/19#newcode721
src/ia32/stub-cache-ia32.cc:721: // Return the register containing the
holder.
On 2010/02/05 09:02:33, Kevin Millikin wrote:
containing ==> containin'
Done.
http://codereview.chromium.org/573009
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
