- Allow parsing of external methods.

- Allow parsing of external methods.
Committed: https://code.google.com/p/dart/source/detail?r=10028

[hausner, 2012-07-30 16:22:41]

LGTM but I am missing a test to make sure an exception is thrown if a missing
"external" is invoked.

I assume this is just experimental, otherwise I would like to see a spec.

As we discussed in person last week, I think the "external" annotation should go
at the end, at the same place where we say "native". That way it's much easier
to parse (less overhead, less lookahead etc) and it's visually less offensive.
In a way, external also similar to native that that the function code is
somewhere else, but instead of C/C++ code, the body is provided in Dart.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser....
runtime/vm/parser.cc:4533: (is_external && (CurrentToken() ==
Token::kSEMICOLON))) {
I don't think you need to check for the semicolon here. All this function needs
to do is checking tokens minimally until we know that we are looking at a
function declaration, not anything else.

[hausner, 2012-07-30 16:25:59]

One more comment.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser....
runtime/vm/parser.cc:2504: 
Can a field be external? If not, this needs to be checked here.

[siva, 2012-07-30 17:12:39]

LGTM.

I agree with Mathias, external could be made similar to native maybe we could
even merge the two:

..... external "asasd"; would be a C/C++ native method.
..... external; would be a dart method that can be patched.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
runtime/vm/compiler.cc:451: false,  // not external
missing '.'

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
runtime/vm/dart_api_impl_test.cc:5390: "  return 4 /* topLevel(2) */;\n"
Why not make this a test for ensuring an exception is thrown when there is a
missing patch instead of commenting out the code with a TODO.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
runtime/vm/raw_object.h:585: bool is_external_;
I think we should make these bools a bit field set to shrink space. Not in this
CL but a different one.

[Ivan Posva, 2012-07-30 18:19:54]

On 2012/07/30 17:12:39, asiva wrote:
> LGTM.
> 
> I agree with Mathias, external could be made similar to native maybe we could
> even merge the two:
> 
> ..... external "asasd"; would be a C/C++ native method.
> ..... external; would be a dart method that can be patched.

Some of the variants you and Matthias were discussed and Gilad decided on the
current version. I trust his judgement in this regard.

I am addressing the other comments and adding tests.

-Ivan

> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
> File runtime/vm/compiler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
> runtime/vm/compiler.cc:451: false,  // not external
> missing '.'
> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
> File runtime/vm/dart_api_impl_test.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
> runtime/vm/dart_api_impl_test.cc:5390: "  return 4 /* topLevel(2) */;\n"
> Why not make this a test for ensuring an exception is thrown when there is a
> missing patch instead of commenting out the code with a TODO.
> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
> File runtime/vm/raw_object.h (right):
> 
>
https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
> runtime/vm/raw_object.h:585: bool is_external_;
> I think we should make these bools a bit field set to shrink space. Not in
this
> CL but a different one.

[Ivan Posva, 2012-07-30 22:33:33]

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/compile...
runtime/vm/compiler.cc:451: false,  // not external
On 2012/07/30 17:12:39, asiva wrote:
> missing '.'

Done.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/dart_ap...
runtime/vm/dart_api_impl_test.cc:5390: "  return 4 /* topLevel(2) */;\n"
On 2012/07/30 17:12:39, asiva wrote:
> Why not make this a test for ensuring an exception is thrown when there is a
> missing patch instead of commenting out the code with a TODO.

Adding a language test for some of this functionality and negative testing.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser....
runtime/vm/parser.cc:2504: 
On 2012/07/30 16:25:59, hausner wrote:
> Can a field be external? If not, this needs to be checked here.

Done.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/parser....
runtime/vm/parser.cc:4533: (is_external && (CurrentToken() ==
Token::kSEMICOLON))) {
On 2012/07/30 16:22:41, hausner wrote:
> I don't think you need to check for the semicolon here. All this function
needs
> to do is checking tokens minimally until we know that we are looking at a
> function declaration, not anything else. 

Done.

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10821076/diff/10001/runtime/vm/raw_obj...
runtime/vm/raw_object.h:585: bool is_external_;
On 2012/07/30 17:12:39, asiva wrote:
> I think we should make these bools a bit field set to shrink space. Not in
this
> CL but a different one.

As far as I know this is something Todd had started work on.