Looks good. Some comments.
https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc
File src/ast.cc (right):
https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc#newcode353
src/ast.cc:353: if (saw_computed_name &&
This is surprising: having seen a computed name previously affects how
the current property is handled? Are you sure this is correct? (I bet we
lack test coverage for this; NeedsHomeObject() is fairly arcane.)
https://codereview.chromium.org/1328603003/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/1328603003/diff/40001/src/flag-definitions.h#newcode704
src/flag-definitions.h:704: DEFINE_BOOL(vector_stores, true, "use
vectors for store ics")
Don't forget to turn this off again :-)
https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedback-vector.cc
File test/cctest/test-feedback-vector.cc (right):
https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedback-vector.cc#newcode419
test/cctest/test-feedback-vector.cc:419: CHECK_EQ(FLAG_vector_stores ? 4
: 2, feedback_vector->ICSlots());
suggestion:
if (FLAG_vector_stores) {
CHECK_EQ(4, feedback_vector->ICSlots());
CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) ==
Code::LOAD_IC);
CHECK( 1
STORE_IC);
CHECK( 2
LOAD_IC);
CHECK( 3
STORE_IC);
} else {
CHECK_EQ(2, feedback_vector->ICSlots());
CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) ==
Code::LOAD_IC);
CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(1)) ==
Code::LOAD_IC);
}
Provides more test coverage now, and is trivial to simplify when
FLAG_vector_stores is removed.
https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedback-vector.cc#newcode453
test/cctest/test-feedback-vector.cc:453:
FeedbackVectorICSlot(FLAG_vector_stores ? 3 : 2)) == Code::CALL_IC);
suggestion:
int slot = 0;
CHECK(...(FeedbackVectorICSlot(slot++)) == Code::CALL_IC);
CHECK(...(FeedbackVectorICSlot(slot++)) == Code::LOAD_IC);
if (FLAG_vector_stores) {
CHECK(...(FeedbackVectorICSlot(slot++)) == Code::STORE_IC);
}
...
https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedback-vector.cc#newcode503
test/cctest/test-feedback-vector.cc:503: // Function f has 3 LoadICs,
one for each o, but the ICs share the same
lolwut?
https://codereview.chromium.org/1328603003/
--
--
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/d/optout.
