Introduce a class encapsulating arguments descriptor arrays.

Introduce a class encapsulating arguments descriptor arrays.


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15727

[Kevin Millikin (Google), 2012-12-05 11:45:23]

I'm about to refactor parameter passing to the inline cache miss runtime
functions.  This will make the refactoring simpler.

[Vyacheslav Egorov (Google), 2012-12-05 12:04:16]

lgtm

https://codereview.chromium.org/11442010/diff/1/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11442010/diff/1/runtime/vm/dart_entry.cc#newc...
runtime/vm/dart_entry.cc:182: int insert_index = kFirstNamedEntryIndex +
(kNamedEntrySize * i);
intptr_t

[srdjan, 2012-12-05 18:27:52]

I will be fixing the issues mentioned below in an upcoming CL.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:33: explicit ArgumentsDescriptor(RawObject* array);
NEVER pass a raw object as argument, only handles.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:39: RawObject* NameAt(intptr_t index) const;
Maybe this should be RawString*?

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:75: const Array& array_;
Add DISALLOW_YADA_YADA

[regis, 2012-12-05 18:39:24]

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:678: ArgumentsDescriptor
arg_desc(arguments.ArgAt(2));
Why did you remove the CheckedHandle here and below?

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:678: ArgumentsDescriptor
arg_desc(arguments.ArgAt(2));
You cannot pass a raw object as parameter.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc#n...
runtime/vm/dart_entry.cc:137: RawObject* ArgumentsDescriptor::NameAt(intptr_t
index) const {
Why not RawString*?

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:33: explicit ArgumentsDescriptor(RawObject* array);
You should never pass a RawObject pointer as parameter.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:36: intptr_t Count() const;
Currently, we name integer variables holding a count of things "num_things" and
their accessors "NumThings()". I would keep this convention for consistency.

[Kevin Millikin (Google), 2012-12-05 19:11:23]

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:678: ArgumentsDescriptor
arg_desc(arguments.ArgAt(2));
On 2012/12/05 18:39:24, regis wrote:
> Why did you remove the CheckedHandle here and below?

Here: it's not removed, the ArgumentsDescriptor constructor does CheckedHandle.

Below: because I couldn't figure out why it was there.  Without any
documentation, the best I could understand is that we use CheckedHandle on
arguments to runtime calls, but we rely on debug-only assertions for values we
read out of the heap with a known (expected) type.

If the rule is that all downcasts are checked also in release builds, I think we
have to change a lot of other code, too.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc#n...
runtime/vm/dart_entry.cc:137: RawObject* ArgumentsDescriptor::NameAt(intptr_t
index) const {
On 2012/12/05 18:39:24, regis wrote:
> Why not RawString*?

To let the caller cast it without allocating an extra handle.  There's no simple
way that I can see to cast a RawObject* to RawString*.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:33: explicit ArgumentsDescriptor(RawObject* array);
On 2012/12/05 18:39:24, regis wrote:
> You should never pass a RawObject pointer as parameter.

Well, that's obviously a qualified NEVER, because I count over a dozen distinct
places that we do that (isolate, object_set, snapshot, symbols, Handle static
functions, etc).

I reasoned as follows: this is just a wrapped handle.  If Array::Handle is
allowed to take a RawArray*, then this should as well.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:36: intptr_t Count() const;
On 2012/12/05 18:39:24, regis wrote:
> Currently, we name integer variables holding a count of things "num_things"
and
> their accessors "NumThings()". I would keep this convention for consistency.

I know bit it drives me nuts.  It directly contradicts the style guide to avoid
abbreviations.  "count" is only one (or fewer) character more than "num" because
you can turn the noun from plural to singular, and it is readable as an English
word unlike "num".

Count reads way better than Num (ArgumentsDescriptor::Num()?) and I'm not sure
this new class needs to be consistent with anything.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:39: RawObject* NameAt(intptr_t index) const;
On 2012/12/05 18:27:52, srdjan wrote:
> Maybe this should be RawString*?

It's too annoying, because we don't have a simple way to cast RawObject* to
RawString*.  It seemed more pleasant here to let the caller handle casting the
handle.

[srdjan, 2012-12-05 19:34:17]

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:678: ArgumentsDescriptor
arg_desc(arguments.ArgAt(2));
On 2012/12/05 19:11:23, kmillikin wrote:
> On 2012/12/05 18:39:24, regis wrote:
> > Why did you remove the CheckedHandle here and below?
> 
> Here: it's not removed, the ArgumentsDescriptor constructor does
CheckedHandle.
> 
> Below: because I couldn't figure out why it was there.  Without any
> documentation, the best I could understand is that we use CheckedHandle on
> arguments to runtime calls, but we rely on debug-only assertions for values we
> read out of the heap with a known (expected) type.
> 
> If the rule is that all downcasts are checked also in release builds, I think
we
> have to change a lot of other code, too.

We need to document when to use CheckedHandle (only in runtime and native
functions when transitioning from compiled code).

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.cc#n...
runtime/vm/dart_entry.cc:137: RawObject* ArgumentsDescriptor::NameAt(intptr_t
index) const {
On 2012/12/05 19:11:23, kmillikin wrote:
> On 2012/12/05 18:39:24, regis wrote:
> > Why not RawString*?
> 
> To let the caller cast it without allocating an extra handle.  There's no
simple
> way that I can see to cast a RawObject* to RawString*.

Florian did something in https://codereview.chromium.org/11444008.

We can do that if it shows up in profile.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:33: explicit ArgumentsDescriptor(RawObject* array);
On 2012/12/05 19:11:23, kmillikin wrote:
> On 2012/12/05 18:39:24, regis wrote:
> > You should never pass a RawObject pointer as parameter.
> 
> Well, that's obviously a qualified NEVER, because I count over a dozen
distinct
> places that we do that (isolate, object_set, snapshot, symbols, Handle static
> functions, etc).
> 
> I reasoned as follows: this is just a wrapped handle.  If Array::Handle is
> allowed to take a RawArray*, then this should as well.

Yes, it is a general rule, it should be deviated from only for very special
reasons.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:36: intptr_t Count() const;
On 2012/12/05 19:11:23, kmillikin wrote:
> On 2012/12/05 18:39:24, regis wrote:
> > Currently, we name integer variables holding a count of things "num_things"
> and
> > their accessors "NumThings()". I would keep this convention for consistency.
> 
> I know bit it drives me nuts.  It directly contradicts the style guide to
avoid
> abbreviations.  "count" is only one (or fewer) character more than "num"
because
> you can turn the noun from plural to singular, and it is readable as an
English
> word unlike "num".
> 
> Count reads way better than Num (ArgumentsDescriptor::Num()?) and I'm not sure
> this new class needs to be consistent with anything.

The most important thing is to be consistent, pick one and stay with it. Let's
pick one, and fix other cases.

https://codereview.chromium.org/11442010/diff/5001/runtime/vm/dart_entry.h#ne...
runtime/vm/dart_entry.h:39: RawObject* NameAt(intptr_t index) const;
On 2012/12/05 19:11:23, kmillikin wrote:
> On 2012/12/05 18:27:52, srdjan wrote:
> > Maybe this should be RawString*?
> 
> It's too annoying, because we don't have a simple way to cast RawObject* to
> RawString*.  It seemed more pleasant here to let the caller handle casting the
> handle.

It is a weird API. We know we can return only RawString.