Comments addressed, new CL uploaded...
https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode4436 src/objects.cc:4436: MaybeObject* getterOk = GetHeap()->undefined_value(); On 2012/05/02 13:30:14, Michael Starzinger wrote:
Can we move the result of "GetHeap()" into a local "heap" variable?
Same goes
for the other methods below.
Done for the occurrences where it is actually used more than once in a possible control flow. Rationale: I'd like to keep the mental context needed for understanding a piece of code as small as possible, and every local variable works against that goal. OTOH, speed considerations might make such a manual CSE sometimes necessary. https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode4649 src/objects.cc:4649: return NewCallbackTrans(name, component, accessor, attributes, accessors); On 2012/05/02 13:30:14, Michael Starzinger wrote:
Can we add an assertion to the top of this method that ASSERT(!accessor->IsMap()) to make sure we are not accidentally
introducing
transitions with this method.
I'll add a slightly stronger assertion, checking that the accessor is either a spec function or undefined. https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode4657 src/objects.cc:4657: MaybeObject* JSObject::CreateFreshAccessor(String* name, On 2012/05/02 13:30:14, Michael Starzinger wrote:
Can we turn this method into a static helper that returns a "MaybeMap"
to
transition to? The call to "set_map()" would be moved into the caller
(i.e.
"DefineFastAccessor"). That way all this complexity would be hidden
from the API
(i.e. JSObject) and there would only be one entry point to define fast
JS-style
accessors.
As discussed offline, let's just simply pass 'this' to keep changes in an atomic way. https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode4709 src/objects.cc:4709: MaybeObject* JSObject::NewCallbackTrans(String* name, On 2012/05/02 13:30:14, Michael Starzinger wrote:
Likewise. Also the full name "NewCallbackTransition" would be better.
It was because of our 80-column limit. :-) Done. https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.h File src/objects.h (right): https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.h#newcode713 src/objects.h:713: class AccessorPair; On 2012/05/02 13:30:14, Michael Starzinger wrote:
Can we alpha-sort this?
Done. https://chromiumcodereview.appspot.com/10238005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
