Pass closure object as first implicit argument to closure functions.

Pass closure object as first implicit argument to closure functions.
Remove code passing captured receiver to native instance closures; instead,
access captured receiver in context, as non-native functions do.
Committed: https://code.google.com/p/dart/source/detail?r=14698

[siva, 2012-11-07 18:56:32]

lgtm with couple of questions
1. index value passed in to Dart_GetNativeArgument
2. count computation in ParameterCountForResolution

http://codereview.chromium.org/11360116/diff/1/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/code_generator.cc#n...
runtime/vm/code_generator.cc:1260: // Receiver parameter has already been
skipped by caller.
We should probably add a comment here indicating why the closure needs to be
passed as a the first argument

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:3942: index++;  // Hide closure object.
This seems to suggest that if one passed in an index of -1
to this function and the arguments are for a static implicit closure then we
will not be reporting an error.

Maybe you should increment the index after the index check
below?

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:3970: // argument count remains unchanged.
This comment should appear outside the if statement right.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h
File runtime/vm/native_arguments.h (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:66: bool AreOfInstanceFunction() const {
I find the name AreOfInstanceFunction() a bit hard to read
maybe ToInstanceFunction().

You don't have to change this I am fine with either.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:102: count--;
I am a little confused by this.
If a function is static and has been implicitly closurised then the closure
object should be hidden and the argument count becomes one less but the check
above seems inverted.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:136: int argc_tag_;  // Encodes argument count and
argument location.
// Encodes argument count and argument location.

Should that be

// Encodes argument count and invoked native call type.

[regis, 2012-11-08 18:08:30]

Thanks!

http://codereview.chromium.org/11360116/diff/1/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/code_generator.cc#n...
runtime/vm/code_generator.cc:1260: // Receiver parameter has already been
skipped by caller.
On 2012/11/07 18:56:32, siva wrote:
> We should probably add a comment here indicating why the closure needs to be
> passed as a the first argument

Done.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:3942: index++;  // Hide closure object.
On 2012/11/07 18:56:32, siva wrote:
> This seems to suggest that if one passed in an index of -1
> to this function and the arguments are for a static implicit closure then we
> will not be reporting an error.
> 
> Maybe you should increment the index after the index check
> below?

Good catch! Incrementing the index after the check would require the upper bound
to be adjusted prior to the check. It is actually simpler to use
Dart_GetNativeArgumentCount here.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:3970: // argument count remains unchanged.
On 2012/11/07 18:56:32, siva wrote:
> This comment should appear outside the if statement right.

Done.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h
File runtime/vm/native_arguments.h (right):

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:66: bool AreOfInstanceFunction() const {
On 2012/11/07 18:56:32, siva wrote:
> I find the name AreOfInstanceFunction() a bit hard to read
> maybe ToInstanceFunction().
> 
> You don't have to change this I am fine with either.

Done.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:102: count--;
On 2012/11/07 18:56:32, siva wrote:
> I am a little confused by this.
> If a function is static and has been implicitly closurised then the closure
> object should be hidden and the argument count becomes one less but the check
> above seems inverted.

Oops. I do not know where the ! is coming from. Good catch.

http://codereview.chromium.org/11360116/diff/1/runtime/vm/native_arguments.h#...
runtime/vm/native_arguments.h:136: int argc_tag_;  // Encodes argument count and
argument location.
On 2012/11/07 18:56:32, siva wrote:
> // Encodes argument count and argument location.
> 
> Should that be
> 
> // Encodes argument count and invoked native call type.

Done.