https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc
File src/code-stubs.cc (right):
https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode434
src/code-stubs.cc:434: if (object->IsUndetectableObject() ||
!object->IsHeapObject()) {
On 2013/04/23 09:27:30, mvstanton wrote:
Could you combine the if with the else above
Done.
https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode437
src/code-stubs.cc:437: if ((types &
CompareNilICStub::kCompareAgainstMonomorphicMap) != 0) {
On 2013/04/23 09:27:30, mvstanton wrote:
And again here.
Done.
https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode906
src/hydrogen.cc:906: ASSERT(!(did_then_ ^ did_else_));
On 2013/04/23 09:27:30, mvstanton wrote:
I'd probably comment this in english: "Make sure we handled both then
and else,
or neither." This took me a minute to figure out.
Done.
https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode1806
src/hydrogen.cc:1806: BuildCheckMap(value, map);
On 2013/04/23 09:27:30, mvstanton wrote:
As I understand it, this BuildCheckMap is important because it embeds
the map as
the first map in reloc handles for the IC. The function is used in 2
ways...in
the inlined case, there is never a replacement done on the map. In the
IC case,
then this map gets replaced when copying from a template.
I could imagine a BuildCompareNilScope or factory method that could
ensure on
teardown that the first handle is the "sentinel map" passed in above.
But that
is probably overkill. Or could an assert be placed right before
BuildCheckMap
that somehow makes sure there are *no* reloc handles in the code
object to this
point? (only in IC case)
In lieu of that I'd highlight the BuildCheckMap with a comment, and
also comment
the declaration in hydrogen.h for this special, interesting concern.
Done.
https://codereview.chromium.org/14367018/diff/17001/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/14367018/diff/17001/src/ic.cc#newcode2806
src/ic.cc:2806: if (Smi::FromInt(object->IsNull() ||
object->IsUndefined())) {
On 2013/04/23 09:27:30, mvstanton wrote:
Do you intend to have Smi::FromInt(... in the if statement? Something
seems
wrong.
Done.
https://codereview.chromium.org/14367018/diff/17001/src/ic.cc#newcode2806
src/ic.cc:2806: if (Smi::FromInt(object->IsNull() ||
object->IsUndefined())) {
On 2013/04/23 09:27:30, mvstanton wrote:
Do you intend to have Smi::FromInt(... in the if statement? Something
seems
wrong.
Done.
https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc
File src/lithium.cc (right):
https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc#newcode332
src/lithium.cc:332: if (block->predecessors()->length() > 1)
can_eliminate = false;
Removed.
On 2013/04/23 09:27:30, mvstanton wrote:
What is it unique about this CL that requires this change? Is it use
of blocks
for push/pop of environment HValues?
https://codereview.chromium.org/14367018/
--
--
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.
