- Allow patching of top-level methods and accessors.

- Allow patching of top-level methods and accessors.
Committed: https://code.google.com/p/dart/source/detail?r=10409

[hausner, 2012-08-07 22:56:35]

This change does not handle function named "patch" correctly.

The more I see this language extension implemented, the more I dislike it. It
messes up the parser in many places, for a very uncommon case.

I suggest that instead of introducing the pseudo-keywords patch and external, we
use the annotation syntax to deal with this, i.e. use annotations like @patch
and @external. That way, there is no possibility of name/keyword collisions, and
the parser can drop all the ugly lookahead and skipping code. The parser will
have to read (and skip) annotations anyway. We could just define a handful of
pre-defined annotation names that the parser then recognizes and interprets.
Simple, easy and much, much cleaner!

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc#n...
runtime/vm/object.cc:5467: Object& entry = Object::Handle(isolate,
Object::null());
No need to explicitly specify null. We don't do it in other places either. I see
it's done in LookupLocalObject, but I still think it's not typical in our
source.

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc#n...
runtime/vm/object.cc:5476: while (!entry.IsNull()) {
Maybe factor out the lookup code into a separate function instead of
copy-pasting it here?

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

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3626: ConsumeToken();
This code can't handle an untyped top-level function called patch(). Or a
top-level function that declares a return value of type 'patch'. Ditto for
external().

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3714: (CurrentLiteral()->Equals("patch"))) {
This does not work for a getter that declares a return type of 'patch' or
'external'.

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3781: ExpectSemicolon();
Can we handle the common case first?

[Ivan Posva, 2012-08-08 00:56:52]

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc#n...
runtime/vm/object.cc:5467: Object& entry = Object::Handle(isolate,
Object::null());
On 2012/08/07 22:56:36, hausner wrote:
> No need to explicitly specify null. We don't do it in other places either. I
see
> it's done in LookupLocalObject, but I still think it's not typical in our
> source.

Done.

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/object.cc#n...
runtime/vm/object.cc:5476: while (!entry.IsNull()) {
On 2012/08/07 22:56:36, hausner wrote:
> Maybe factor out the lookup code into a separate function instead of
> copy-pasting it here?

Done.

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

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3626: ConsumeToken();
On 2012/08/07 22:56:36, hausner wrote:
> This code can't handle an untyped top-level function called patch(). Or a
> top-level function that declares a return value of type 'patch'. Ditto for
> external().
> 

external is a pseudo-keyword -> not a problem
A type patch is only a problem in patch files. Adding a test for non-patch
files.

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3714: (CurrentLiteral()->Equals("patch"))) {
On 2012/08/07 22:56:36, hausner wrote:
> This does not work for a getter that declares a return type of 'patch' or
> 'external'.
ditto.

https://chromiumcodereview.appspot.com/10824209/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3781: ExpectSemicolon();
On 2012/08/07 22:56:36, hausner wrote:
> Can we handle the common case first?

Then

external get a() {}

would pass without error, no?

[hausner, 2012-08-08 17:15:12]

LGTM, but I still don't like it.