On 2013/09/09 15:34:43, Michael Starzinger wrote:
LGTM with final round of comments.
https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1219
src/compiler.cc:1219: return result; // return null.
nit: Can we just write down "Handle<Code>::null()" here? Much easier to
read
and
probably same or even better code.
Done.
https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1244
src/compiler.cc:1244: return result;
Likewise.
Done.
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.cc#newcode1495
src/hydrogen-instructions.cc:1495: // TODO(titzer): Implement me!
Hmm, can we, implement this? It was implemented before, is there a reason
you
removed it?
I think I removed it several weeks back on accident when I removed all the
informative definition crap. It's back now!
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.h#newcode5001
src/hydrogen-instructions.h:5001: HEnvironment *environment() { return
environment_; }
I still don't like the fact that we preserve the full environment here. At
least
we are making a copy when we create the OSR-graph now, so I am fine with
landing
as it is.
Ack.
https://codereview.chromium.org/21340002/diff/21001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/21340002/diff/21001/src/objects.cc#newcode9327
src/objects.cc:9327: ASSERT(result !=
Isolate::Current()->has_pending_exception());
Please keep using info->isolate() here, Isolate::Current() is on death
row.
Done.
https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8417
src/runtime.cc:8417: ASSERT(frame->function() == *function);
nit: Move up one line.
Done.
https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8595
src/runtime.cc:8595: // which the unoptimized code can jump into.
nit: Drop one of the "which".
Done.
https://codereview.chromium.org/21340002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
