Cool stuff! Just a few comments.
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()) {
Could you combine the if with the else above
https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode437
src/code-stubs.cc:437: if ((types &
CompareNilICStub::kCompareAgainstMonomorphicMap) != 0) {
And again here.
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_));
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.
https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode1806
src/hydrogen.cc:1806: BuildCheckMap(value, map);
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.
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())) {
Do you intend to have Smi::FromInt(... in the if statement? Something
seems wrong.
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;
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/diff/17001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/14367018/diff/17001/src/objects.h#newcode4783
src/objects.h:4783: STATIC_ASSERT(ExtraICStateField::kShift ==
ExtendedExtraICStateField::kShift);
This is getting pretty hairy. I'm glad for the static asserts!
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.
