Runtime version of declarative native accessors.

Runtime version of declarative native accessors.

R=svenpanne@chromium.org
BUG=

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

[Sven Panne, 2013-02-19 08:41:16]

First round of comments...

https://codereview.chromium.org/12297012/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/12297012/diff/1/include/v8.h#newcode1569
include/v8.h:1569: struct AccessorDescriptor {
I'm not totally happy with the way this algebraic data type is encoded. Let's
discuss this offline, there are *many* ways of doing it...

Regardless how this finally looks, we need some serious documentation via
comments here!

https://codereview.chromium.org/12297012/diff/1/include/v8.h#newcode4646
include/v8.h:4646: struct DescriptorVisitorHelper {
Why do we need all this stuff here in the external header? (Basically 2
questions: Why at all? Why here?)

https://codereview.chromium.org/12297012/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/12297012/diff/1/src/api.cc#newcode1128
src/api.cc:1128: FACTORY->NewExecutableAccessorInfo();
Don't use FACTORY, get the Isolate via e.g. the name and the factory from it. In
general, never use ISOLATE/FACTORY/HEAP/Isolate::Current.

https://codereview.chromium.org/12297012/diff/1/src/api.cc#newcode1143
src/api.cc:1143: i::Isolate* isolate) {
You don't need an explicit Isolate, just retrieve it from e.g. the name.

https://codereview.chromium.org/12297012/diff/1/src/api.cc#newcode1150
src/api.cc:1150: FACTORY->NewDeclaredAccessorInfo();
Use isolate->factory() here.

https://codereview.chromium.org/12297012/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/12297012/diff/1/src/factory.cc#newcode367
src/factory.cc:367: Handle<DeclaredAccessorDescriptor> descriptor =
Naming this doesn't buy us anything, just "return" (it's cleaner ;-).

https://codereview.chromium.org/12297012/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/12297012/diff/1/src/objects-inl.h#newcode5505
src/objects-inl.h:5505: template<int size>
I'm again not sure if we need this massive machinery. And even if we need it,
the header doesn't feel like the right place.

https://codereview.chromium.org/12297012/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode239
src/objects.cc:239: return *isolate->factory()->NewNumberFromUint(value);
Using Handles in non-handlified code is not GC safe, so you can never use any
handlified factory methods from MaybeObject-land.

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode249
src/objects.cc:249: return *isolate->factory()->NewNumber(value);
Same here...

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode253
src/objects.cc:253: return *isolate->factory()->NewNumber(value);
Same here...

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode259
src/objects.cc:259: return *isolate->factory()->NewNumberFromInt(int32_value);
Same here...

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode314
src/objects.cc:314: default:
No default cases, explicitly handle all cases.

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode334
src/objects.cc:334: default:
No default cases, explicitly handle all cases.

https://codereview.chromium.org/12297012/diff/1/src/objects.cc#newcode13727
src/objects.cc:13727: class AccessorDescriptorLengthCounter {
Very unclear what this does and why it is needed...

https://codereview.chromium.org/12297012/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12297012/diff/1/src/objects.h#newcode8384
src/objects.h:8384: void VisitError() { CHECK(false); }
Hmmm, unless I misunderstood things, this seems to be an indicator that this
class should actually be abstract.

https://codereview.chromium.org/12297012/diff/1/src/objects.h#newcode8423
src/objects.h:8423: DECL_ACCESSORS(serialized_descriptor, ByteArray)
Why do we need it? It's unclear to me, which is an indicator that at least a
comment might be appropriate.

https://codereview.chromium.org/12297012/diff/1/test/cctest/SConscript
File test/cctest/SConscript (right):

https://codereview.chromium.org/12297012/diff/1/test/cctest/SConscript#newcode67
test/cctest/SConscript:67: 'test-declarative-accessors.cc',
I thought our SCons support is dead. Perhaps this file should be killed, check
with jkummerow@.

[drcarney, 2013-03-06 15:43:15]

sven, the last patch has been hanging around for a while. it's ready for review
again.  Danno wants to land it when it clears review, but maybe hide the v8.h
parts behind a flag or something...

[Sven Panne, 2013-03-07 08:18:48]

LGTM with a few nits.

https://codereview.chromium.org/12297012/diff/8001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/12297012/diff/8001/include/v8.h#newcode1612
include/v8.h:1612: bool SetAccessor(Handle<String> name,
Add some kind of warning comment that this part of the API is in a state of
flux.

https://codereview.chromium.org/12297012/diff/8001/include/v8.h#newcode2460
include/v8.h:2460: bool SetAccessor(Handle<String> name,
Same here...

https://codereview.chromium.org/12297012/diff/8001/include/v8.h#newcode2604
include/v8.h:2604: static Local<RawOperationDescriptor> New(Isolate* isolate,
A more descriptive name indicating that the operation is about accessing an
internal field would be nice instead of plain "New".

https://codereview.chromium.org/12297012/diff/8001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/12297012/diff/8001/src/objects.cc#newcode176
src/objects.cc:176: Isolate* isolate) {
Passing just the Heap, not the Isolate in its full glory, would be clearer and
reduces the coupling a tiny bit. Same for the other helper functions below.

https://codereview.chromium.org/12297012/diff/8001/src/objects.cc#newcode199
src/objects.cc:199: return result ? isolate->heap()->true_value()
return isolate->heap()->ToBoolean(result);

https://codereview.chromium.org/12297012/diff/8001/src/objects.cc#newcode211
src/objects.cc:211: return result ? isolate->heap()->true_value()
See above

https://codereview.chromium.org/12297012/diff/8001/src/objects.cc#newcode244
src/objects.cc:244: return result ? isolate->heap()->true_value()
See above

[dcarney, 2013-03-07 11:43:10]

Committed manually as r13856 (presubmit successful).