more to come....
http://codereview.chromium.org/8166017/diff/3001/src/heap.h File src/heap.h (right): http://codereview.chromium.org/8166017/diff/3001/src/heap.h#newcode204 src/heap.h:204: "KeyedStoreElmMonoTransSmiObject") \ Use the full symbol name http://codereview.chromium.org/8166017/diff/3001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1600 src/ic.cc:1600: if (ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC || How about just create a variable "use_monomorphic_stub" that gets set if ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC is true, or in a separate if for IsMonomorphicButMissingTransition, so that you don't have to re-test ic_state == MONOMORPHIC && stub_kind > STORE_NO_TRANSITION http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1603 src/ic.cc:1603: target()->ContainsMap(receiver->map()))) { Can you put these into a predicate or something that makes it clear what the test is actually trying to accomplish? Like MonomorphicButMissingTransition. Whatever. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1604 src/ic.cc:1604: Map* existing_additional_map = NULL; smi_only_or_double_map? Or at least comment the name. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1759 src/ic.cc:1759: bool force_generic) { stub_kind? http://codereview.chromium.org/8166017/diff/3001/src/ic.h File src/ic.h (right): http://codereview.chromium.org/8166017/diff/3001/src/ic.h#newcode345 src/ic.h:345: enum StubKind { LOAD, STORE_NO_TRANSITION, STORE_TRANSITION_SMI_TO_OBJECT, nit: list these vertically, they are much easier to read. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2223 src/objects.cc:2223: ASSERT(safe_to_add_transition); comment about this assert? http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2280 src/objects.cc:2280: if (current_map->elements_kind() == FAST_SMI_ONLY_ELEMENTS && can you put this in "from_kind" and rename the other one to "to_kind" http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2346 src/objects.cc:2346: } I think you need a test that covers these code paths. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode7536 src/objects.cc:7536: // TODO(jkummerow): Consider refactoring this to share some code. Do you still need this? http://codereview.chromium.org/8166017/diff/3001/src/objects.h File src/objects.h (right): http://codereview.chromium.org/8166017/diff/3001/src/objects.h#newcode3748 src/objects.h:3748: bool ContainsMap(Map* map); Do you still need this? http://codereview.chromium.org/8166017/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
