Addressed comments. PTAL.
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());
On 2012/06/29 16:31:36, Jakob wrote:
Either enable or remove this, please.
Done.
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"
On 2012/06/29 16:31:36, Jakob wrote:
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.
Done.
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
On 2012/06/29 16:31:36, Jakob wrote:
nit: missing space after '.'.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode1967
src/objects-inl.h:1967: // SLOW_ASSERT(IsSortedNoDuplicates());
On 2012/06/29 16:31:36, Jakob wrote:
Please either (fix and) enable this or remove it.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode2013
src/objects-inl.h:2013: void DescriptorArray::set_transitions(
On 2012/06/29 16:31:36, Jakob wrote:
nit: for declarations, each argument on its own line (if they don't
all fit on
the first), please.
Done.
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.
On 2012/06/29 16:31:36, Jakob wrote:
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.
Done.
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
On 2012/06/29 16:31:36, Jakob wrote:
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?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode3577
src/objects-inl.h:3577: if (transitions_array == transitions()) {
On 2012/06/29 16:31:36, Jakob wrote:
You can just do ASSERT(transitions_array != transitions()) here.
Done.
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));
On 2012/06/29 16:31:36, Jakob wrote:
While you're at it, you can get rid of the cast here by declaring
"FixedArray*
values" in the first place.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1585
src/objects.cc:1585: // We have accomplished the main goal, so return
success.
On 2012/06/29 16:31:36, Jakob wrote:
Huh?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1637
src/objects.cc:1637: Object* new_map;
On 2012/06/29 16:31:36, Jakob wrote:
Why not "Map* new_map"?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1654
src/objects.cc:1654: // Do not add CONSTANT_TRANSITIONS to global
objects
On 2012/06/29 16:31:36, Jakob wrote:
update comment?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1659
src/objects.cc:1659: // Add a CONSTANT_TRANSITION descriptor to the old
map,
On 2012/06/29 16:31:36, Jakob wrote:
update comment?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1668
src/objects.cc:1668: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
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.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1671
src/objects.cc:1671: // We have accomplished the main goal, so return
success.
On 2012/06/29 16:31:36, Jakob wrote:
I'm not convinced that returning success here is reasonable, but let's
leave it
as it is in this CL.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1675
src/objects.cc:1675: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
see above.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1832
src/objects.cc:1832: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
Again, please wrap in {}.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode1839
src/objects.cc:1839: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
Again.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode2193
src/objects.cc:2193: static void LookupTransition(Map* map,
On 2012/06/29 16:31:36, Jakob wrote:
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).
Done.
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);
On 2012/06/29 16:31:36, Jakob wrote:
Wrap in {} please.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode2938
src/objects.cc:2938: // Is transition to CONSTANT_FUNCTION
On 2012/06/29 16:31:36, Jakob wrote:
nit: missing full stop.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4287
src/objects.cc:4287: // TODO(verwaest) clean test
On 2012/06/29 16:31:36, Jakob wrote:
?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4621
src/objects.cc:4621: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
Wrap in {} please.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4624
src/objects.cc:4624: // We have accomplished the main goal, so return
success.
On 2012/06/29 16:31:36, Jakob wrote:
Comment is wrong.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4629
src/objects.cc:4629: MaybeObject* transition_added =
map1->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
again, {} please.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4683
src/objects.cc:4683: MaybeObject* maybe_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
{}
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode4688
src/objects.cc:4688: MaybeObject* transition_added =
map2->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
{}
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5060
src/objects.cc:5060: *TransitionArrayHeader() = Smi::FromInt(0);
On 2012/06/29 16:31:36, Jakob wrote:
nit: {} please.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5098
src/objects.cc:5098: break;
On 2012/06/29 16:31:36, Jakob wrote:
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.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode5923
src/objects.cc:5923: // Copy the descriptors, filtering out transitions
and null descriptors,
On 2012/06/29 16:31:36, Jakob wrote:
update comment?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode7350
src/objects.cc:7350: return ClearBackPointer(heap,
t->GetValue(transition_index));
On 2012/06/29 16:31:36, Jakob wrote:
nit: indentation is off
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode7355
src/objects.cc:7355: return getter && setter;
No. Getter and setter are booleans indicating that their related
component was cleared. Only if both components are cleared we can clear
the whole pair.
On 2012/06/29 16:31:36, Jakob wrote:
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);
On 2012/06/29 16:31:36, Jakob wrote:
I guess you forgot to remove these declarations?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode2070
src/objects.h:2070: PrintElements(stdout);
On 2012/06/29 16:31:36, Jakob wrote:
Don't you mean PrintTransitions(stdout) here?
Done.
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");
On 2012/06/29 16:31:36, Jakob wrote:
nit: callbacks is one word
Done.
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"
On 2012/06/29 16:31:36, Jakob wrote:
Again, #including objects.h is unnecessary if you #include
objects-inl.h too.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode82
src/transitions-inl.h:82: Map* transition_map, WriteBarrierMode mode) {
On 2012/06/29 16:31:36, Jakob wrote:
one line per argument, please.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode83
src/transitions-inl.h:83: ASSERT(this != NULL);
On 2012/06/29 16:31:36, Jakob wrote:
can this happen?
Done.
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
On 2012/06/29 16:31:36, Jakob wrote:
nit: missing colon ("// TODO(verwaest): ...")
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#newcode175
src/transitions-inl.h:175: if (this == NULL) return kNotFound;
On 2012/06/29 16:31:36, Jakob wrote:
can this happen?
Done.
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"
On 2012/06/29 16:31:36, Jakob wrote:
transitions-inl.h is enough.
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#newcode85
src/transitions.cc:85: if (this == NULL) {
On 2012/06/29 16:31:36, Jakob wrote:
can this happen?
Done.
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
No, the comment was out of date. It's length() - 1, because there's the
elements transition. It'll be - 2 once the prototype transition is in;
and - 3 once the descriptor array is in as well.
On 2012/06/29 16:31:36, Jakob wrote:
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);
On 2012/06/29 16:31:36, Jakob wrote:
Set{Key,Value}Unchecked() -- what do we need these for?
Done.
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
On 2012/06/29 16:31:36, Jakob wrote:
who's "they"?
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode120
src/transitions.h:120: // Is the transition array sorted and without
duplicates?
On 2012/06/29 16:31:36, Jakob wrote:
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.)
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode151
src/transitions.h:151: static inline void NoIncrementalWriteBarrierSwap(
On 2012/06/29 16:31:36, Jakob wrote:
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.)
Done.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode155
src/transitions.h:155: inline void
NoIncrementalWriteBarrierSwapTransitions(
On 2012/06/29 16:31:36, Jakob wrote:
I don't see where this is used.
Done.
https://chromiumcodereview.appspot.com/10697015/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
