Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10535180: Allow implicit 'close your eyes' of native methods. This will hopefully fix issue 3466. (Closed)

Created:
8 years, 6 months ago by siva
Modified:
8 years, 6 months ago
Reviewers:
regis, srdjan
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Allow implicit 'close your eyes' of native methods. This will hopefully fix issue 3466. - when resolving the native method which has been closurized account for the 'this' argument in the count of arguments. - explicitly copy the 'this' parameter in the closure context as the first parameter before calling the native function. This is done by piggy backing on the CopyParameters code used for functions with optional parameters. Committed: https://code.google.com/p/dart/source/detail?r=8816

Patch Set 1 #

Patch Set 2 : #

Total comments: 16

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+615 lines, -234 lines) Patch
M vm/ast.h View 1 2 3 chunks +8 lines, -2 lines 0 comments Download
M vm/code_generator_ia32.cc View 1 2 5 chunks +93 lines, -73 lines 0 comments Download
M vm/code_generator_test.cc View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M vm/code_patcher_ia32_test.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M vm/code_patcher_x64_test.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M vm/dart_api_impl_test.cc View 1 2 1 chunk +285 lines, -0 lines 0 comments Download
M vm/flow_graph_compiler_ia32.cc View 1 2 3 chunks +87 lines, -72 lines 0 comments Download
M vm/flow_graph_compiler_x64.cc View 1 2 4 chunks +88 lines, -72 lines 0 comments Download
M vm/intermediate_language.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M vm/intermediate_language_ia32.cc View 1 2 1 chunk +7 lines, -3 lines 0 comments Download
M vm/intermediate_language_x64.cc View 1 2 1 chunk +7 lines, -3 lines 0 comments Download
M vm/object.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M vm/object.cc View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M vm/parser.cc View 1 2 4 chunks +16 lines, -5 lines 0 comments Download
M vm/raw_object.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M vm/raw_object_snapshot.cc View 1 2 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
siva
8 years, 6 months ago (2012-06-15 01:16:24 UTC) #1
regis
LGTM, but static native closures are not handled properly. https://chromiumcodereview.appspot.com/10535180/diff/19/vm/ast.h File vm/ast.h (right): https://chromiumcodereview.appspot.com/10535180/diff/19/vm/ast.h#newcode1497 vm/ast.h:1497: ...
8 years, 6 months ago (2012-06-15 17:13:34 UTC) #2
siva
8 years, 6 months ago (2012-06-16 00:25:43 UTC) #3 
Thanks for the review.

Fixed the static native function case and added a test case for that.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/ast.h
File vm/ast.h (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/ast.h#newcode1497
vm/ast.h:1497: bool is_implicit_native_closure)
I named it is_native_instance_closure

On 2012/06/15 17:13:34, regis wrote:
> You could drop "implicit", since there is no way of declaring an explicit
native
> closure. However, you need to add "instance": is_instance_native_closure or
> is_native_instance_closure.

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/code_generator_ia3...
File vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/code_generator_ia3...
vm/code_generator_ia32.cc:377: function.is_native() &&
function.IsImplicitClosureFunction();
Nice catch. Thanks for finding this.

On 2012/06/15 17:13:34, regis wrote:
> You should be using IsImplicitInstanceClosureFunction().

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/dart_api_impl_test.cc
File vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/dart_api_impl_test...
vm/dart_api_impl_test.cc:5260: "  int foo2(int i) native \"NativeFoo2\";\n"
On 2012/06/15 17:13:34, regis wrote:
> Add a test for a static native.

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/flow_graph_compile...
File vm/flow_graph_compiler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/flow_graph_compile...
vm/flow_graph_compiler_ia32.cc:86: function.is_native() &&
function.IsImplicitClosureFunction();
On 2012/06/15 17:13:34, regis wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/flow_graph_compile...
File vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/flow_graph_compile...
vm/flow_graph_compiler_x64.cc:652: function.is_native() &&
function.IsImplicitClosureFunction();
On 2012/06/15 17:13:34, regis wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/intermediate_langu...
File vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/intermediate_langu...
vm/intermediate_language.h:795: bool is_implicit_native_closure() const {
On 2012/06/15 17:13:34, regis wrote:
> is_native_instance_closure?

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/parser.cc
File vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/parser.cc#newcode147
vm/parser.cc:147: function().is_native() &&
function().IsImplicitClosureFunction();
On 2012/06/15 17:13:34, regis wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10535180/diff/19/vm/parser.cc#newcode3966
vm/parser.cc:3966: const bool is_implicit_closure =
func.IsImplicitClosureFunction();
On 2012/06/15 17:13:34, regis wrote:
> const bool is_instance_closure = func.IsImplicitInstanceClosureFunction();

Done.

Powered by Google App Engine
This is Rietveld 408576698