I have some comments. These are for patch set 1; haven't looked at patch
set 2
in detail yet.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc#newcode4994
src/hydrogen.cc:4994: // ASSERT(lookup->IsMapTransition());
Either enable or remove this, please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode50
src/objects-inl.h:50: #include "transitions.h"
As an exception from not relying on indirect #includes, we usually don't
#include both X.h and X-inl.h anywhere, as we know for sure that X-inl.h
#includes X.h.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode1920
src/objects-inl.h:1920: // Perform a binary search in a fixed array.Low
and high are entry indices. If
nit: missing space after '.'.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode1967
src/objects-inl.h:1967: // SLOW_ASSERT(IsSortedNoDuplicates());
Please either (fix and) enable this or remove it.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode2013
src/objects-inl.h:2013: void DescriptorArray::set_transitions(
nit: for declarations, each argument on its own line (if they don't all
fit on the first), please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode3521
src/objects-inl.h:3521: // transition array that will have place for an
element transition.
nit:
// If the descriptor array does not have a transition array, install a
new
// empty transition array that will have room for an elements
transition.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode3547
src/objects-inl.h:3547: // If the map is using the empty descriptor
array, install a new empty
This comment is confusing. Maybe something like "If the map does not
have a descriptor array, create a new empty descriptor array."?
Also, why is this not called from AllowElementsTransition as a
preparational step?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode3577
src/objects-inl.h:3577: if (transitions_array == transitions()) {
You can just do ASSERT(transitions_array != transitions()) here.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc
File src/objects.cc (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1501
src/objects.cc:1501: set_properties(FixedArray::cast(values));
While you're at it, you can get rid of the cast here by declaring
"FixedArray* values" in the first place.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1585
src/objects.cc:1585: // We have accomplished the main goal, so return
success.
Huh?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1637
src/objects.cc:1637: Object* new_map;
Why not "Map* new_map"?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1654
src/objects.cc:1654: // Do not add CONSTANT_TRANSITIONS to global
objects
update comment?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1659
src/objects.cc:1659: // Add a CONSTANT_TRANSITION descriptor to the old
map,
update comment?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1668
src/objects.cc:1668: MaybeObject* maybe_new_transitions =
Not sure how important it is, but so far we've been pretty consistent in
putting MaybeObjects into their own {}-block. Let's do that here (and
below) too.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1671
src/objects.cc:1671: // We have accomplished the main goal, so return
success.
I'm not convinced that returning success here is reasonable, but let's
leave it as it is in this CL.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1675
src/objects.cc:1675: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
see above.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1832
src/objects.cc:1832: MaybeObject* maybe_new_transitions =
Again, please wrap in {}.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1839
src/objects.cc:1839: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
Again.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode2193
src/objects.cc:2193: static void LookupTransition(Map* map,
Why is this a local static function? It would be more consistent if it
were a method of the Map class (or even inlined into
LookupTransitionOrDescriptor).
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode2307
src/objects.cc:2307: MaybeObject* added_elements =
this->set_elements_transition_map(next_map);
Wrap in {} please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode2938
src/objects.cc:2938: // Is transition to CONSTANT_FUNCTION
nit: missing full stop.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4287
src/objects.cc:4287: // TODO(verwaest) clean test
?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4621
src/objects.cc:4621: MaybeObject* maybe_new_transitions =
Wrap in {} please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4624
src/objects.cc:4624: // We have accomplished the main goal, so return
success.
Comment is wrong.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4629
src/objects.cc:4629: MaybeObject* transition_added =
map1->set_transitions(new_transitions);
again, {} please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4683
src/objects.cc:4683: MaybeObject* maybe_transitions =
{}
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4688
src/objects.cc:4688: MaybeObject* transition_added =
map2->set_transitions(new_transitions);
{}
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5060
src/objects.cc:5060: *TransitionArrayHeader() = Smi::FromInt(0);
nit: {} please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5098
src/objects.cc:5098: break;
This doesn't look right. Did you mean "continue"? As an alternative,
remove this statement, and put the "We definitely have no map
transition" part into an "else {...}" block.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5923
src/objects.cc:5923: // Copy the descriptors, filtering out transitions
and null descriptors,
update comment?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode7350
src/objects.cc:7350: return ClearBackPointer(heap,
t->GetValue(transition_index));
nit: indentation is off
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode7355
src/objects.cc:7355: return getter && setter;
Shouldn't this be "getter || setter"?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h
File src/objects.h (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode1198
src/objects.h:1198: inline Address FieldAddress(int offset);
I guess you forgot to remove these declarations?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode2070
src/objects.h:2070: PrintElements(stdout);
Don't you mean PrintTransitions(stdout) here?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc
File src/property.cc (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc#newcode95
src/property.cc:95: PrintF(out, " -type = call backs transition\n");
nit: callbacks is one word
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h
File src/transitions-inl.h (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode31
src/transitions-inl.h:31: #include "objects.h"
Again, #including objects.h is unnecessary if you #include objects-inl.h
too.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode82
src/transitions-inl.h:82: Map* transition_map, WriteBarrierMode mode) {
one line per argument, please.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode83
src/transitions-inl.h:83: ASSERT(this != NULL);
can this happen?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode144
src/transitions-inl.h:144: // TODO(verwaest) Currently details are
always taken from the getter if it
nit: missing colon ("// TODO(verwaest): ...")
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode175
src/transitions-inl.h:175: if (this == NULL) return kNotFound;
can this happen?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc
File src/transitions.cc (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#newcode32
src/transitions.cc:32: #include "transitions.h"
transitions-inl.h is enough.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#newcode85
src/transitions.cc:85: if (this == NULL) {
can this happen?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h
File src/transitions.h (right):
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode46
src/transitions.h:46: // [length() - 2] Last transition
Is that "length() - 2" because kTransitionSize == 2? In that case, I'd
write "[length() - kTransitionSize]" instead.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode57
src/transitions.h:57: inline Object** GetKeySlot(int transition_number);
Set{Key,Value}Unchecked() -- what do we need these for?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode78
src/transitions.h:78: // Copy the transition array, inserting a new
transition. While adding, they
who's "they"?
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode120
src/transitions.h:120: // Is the transition array sorted and without
duplicates?
nit: this comment is kinda unnecessary, as it doesn't add information
beyond the function name. Feel free to just drop it.
(Same for the next two methods.)
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode151
src/transitions.h:151: static inline void NoIncrementalWriteBarrierSwap(
I don't see where this is used. If the intention was to forbid its use,
please indicate that in a comment. (You can remove the uninformative
existing comment in turn.)
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode155
src/transitions.h:155: inline void
NoIncrementalWriteBarrierSwapTransitions(
I don't see where this is used.
https://chromiumcodereview.appspot.com/10697015/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
