Cleaned up setting of accessors.

Cleaned up setting of accessors.

This CL is an intermediate step only, in the end we need to have a single
DefineOrRedefineAccessorProperty call for a single Object.defineProperty
call. Currently we can end up making two such calls, making the necessary access
checks extremely ugly and hard (impossible?) to get right for complete spec
conformance.

The bulk of the change is quite mechanical:

 * Prepare an AccessorPair *before* we add it to our data structures,
   eliminating the previous voodoo-like threading of a placeholder.

 * The previous item makes it possible to activate our check that we do not
   share AccessorPairs by accident.

 * Split a monster method into 2 quite unrelated methods.

 * Use templated To method in a few places.

Committed: https://code.google.com/p/v8/source/detail?r=10788

[Michael Starzinger, 2012-02-22 10:24:38]

LGTM.

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc#newcode4418
src/objects.cc:4418: return GetHeap()->undefined_value();
I prefer having the result of GetHeap() in a local "heap" variable. It might not
matter much performance-wise and my opinion about that is not very strong. So I
leave the decision up to you.

[Sven Panne, 2012-02-22 10:52:18]

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc#newcode4418
src/objects.cc:4418: return GetHeap()->undefined_value();
On 2012/02/22 10:24:38, Michael Starzinger wrote:
> I prefer having the result of GetHeap() in a local "heap" variable. It might
not
> matter much performance-wise and my opinion about that is not very strong. So
I
> leave the decision up to you.

I don't think that one possible additional memory access will really hurt us
here in these expensive runtime functions, so I'd like to leave it as it is,
preferring conciseness over a nano-optimization. A world premiere in v8 code!
;-)

[fschneider, 2012-02-24 10:33:17]

dbc:

http://codereview.chromium.org/9428026/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4418
src/objects.cc:4418: return GetHeap()->undefined_value();
On 2012/02/22 10:52:18, Sven Panne wrote:
> On 2012/02/22 10:24:38, Michael Starzinger wrote:
> > I prefer having the result of GetHeap() in a local "heap" variable. It might
> not
> > matter much performance-wise and my opinion about that is not very strong.
So
> I
> > leave the decision up to you.
> 
> I don't think that one possible additional memory access will really hurt us
> here in these expensive runtime functions, so I'd like to leave it as it is,
> preferring conciseness over a nano-optimization. A world premiere in v8 code!
> ;-)

I don't buy the conciseness argument. heap->undefined_value() results in shorter
lines.

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4479
src/objects.cc:4479: return GetHeap()->undefined_value();
return SetPropertyCallback(name, obj, attributes);

would be more concise.

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4493
src/objects.cc:4493: return GetHeap()->undefined_value();
return SetPropertyCallback(name, accessors, attributes);

would be more concise.

[Sven Panne, 2012-02-24 11:47:17]

http://codereview.chromium.org/9428026/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4418
src/objects.cc:4418: return GetHeap()->undefined_value();
On 2012/02/24 10:33:17, fschneider wrote:
> [...] I don't buy the conciseness argument. heap->undefined_value() results in
shorter lines.

It's not about shorter lines, it's all about having a smaller "mental context"
one has to carry around in one's brain when  trying to understand code, and
every local variable adds to this mental context. In the end it's all a
judgement call...

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4453
src/objects.cc:4453: { MaybeObject* maybe_ok = SetElementCallback(index,
accessors, attributes);
I'll apply the same simplification here...

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4479
src/objects.cc:4479: return GetHeap()->undefined_value();
On 2012/02/24 10:33:17, fschneider wrote:
> return SetPropertyCallback(name, obj, attributes);
> 
> would be more concise.

See remark below.

http://codereview.chromium.org/9428026/diff/1/src/objects.cc#newcode4493
src/objects.cc:4493: return GetHeap()->undefined_value();
On 2012/02/24 10:33:17, fschneider wrote:
> return SetPropertyCallback(name, accessors, attributes);
> 
> would be more concise.

Good point, I'll submit a separate CL for this mini-cleanup, although I think
that I'll have to change this code soon, anyway (to make setting of callbacks
more "atomic", in a way like Michael already has done it for data properties).

[Michael Starzinger, 2012-02-24 12:44:09]

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/9428026/diff/1/src/objects.cc#newcode4418
src/objects.cc:4418: return GetHeap()->undefined_value();
On 2012/02/24 11:47:17, Sven Panne wrote:
> On 2012/02/24 10:33:17, fschneider wrote:
> > [...] I don't buy the conciseness argument. heap->undefined_value() results
in
> shorter lines.
> 
> It's not about shorter lines, it's all about having a smaller "mental context"
> one has to carry around in one's brain when  trying to understand code, and
> every local variable adds to this mental context. In the end it's all a
> judgement call...

Knowing that "heap" refers to the one and only heap object in the current
isolate is constant in my brain. So no additional pressure on my mental context
in this case. Having to read "GetHeap()" makes me start to wonder: "Hang on, it
needs the map to get the heap, is the map in a consistent state? How much does
it cost to take that indirection?", that's much more pressure on my mental
context.

Just my two cents.