Simplify parsing of 'new' operator.

Simplify parsing of 'new' operator.
Fix checking of default factory type parameters (add new tests).
Start cleanup of class resolution code in parser and finalizer.
Committed: https://code.google.com/p/dart/source/detail?r=3632

[srdjan, 2012-01-26 22:52:02]

DBC

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

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.h#new...
runtime/vm/parser.h:227: TypeResolution type_resolution);
IMO, it looks better to have the result 'type' at the end of the argument list.

[hausner, 2012-01-26 23:04:29]

LGTM, with one question about visibility/scope of type parameters (see below).

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

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:1923: // shadowed by a type parameter in scope.
Are type parameters always "in scope" when we parse qualified identifiers? I
thought it was more complicated, e.g. type parameters not being in scope when
parsing a static function in a class.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6647: // This was already checked by ParseQualIdent, unless
is_top_level_.
It's not immediately clear what "This" in the comment above refers to. I presume
it refers to looking up whether the name is a type parameter.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6652: ErrorMsg(type_pos,
Same comment as bove: type parameters are not visible everywhere, I think.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7110: // the parsed type.
I would switch the two parts of the sentence around for better readability: "The
constructor class ...., unless the parsed type is ... (checked below)."

[regis, 2012-01-26 23:45:17]

Thanks both!

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

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:1923: // shadowed by a type parameter in scope.
On 2012/01/26 23:04:29, hausner wrote:
> Are type parameters always "in scope" when we parse qualified identifiers? I
> thought it was more complicated, e.g. type parameters not being in scope when
> parsing a static function in a class.

Correct. When type parameters are not in scope, TypeParametersScopeClass()
returns null.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6647: // This was already checked by ParseQualIdent, unless
is_top_level_.
On 2012/01/26 23:04:29, hausner wrote:
> It's not immediately clear what "This" in the comment above refers to. I
presume
> it refers to looking up whether the name is a type parameter.

Correct. I made it clearer by merging both comments.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6652: ErrorMsg(type_pos,
On 2012/01/26 23:04:29, hausner wrote:
> Same comment as bove: type parameters are not visible everywhere, I think.

This is taken care of by TypeParametersScopeClass() returning null if not in
scope.

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7110: // the parsed type.
On 2012/01/26 23:04:29, hausner wrote:
> I would switch the two parts of the sentence around for better readability:
"The
> constructor class ...., unless the parsed type is ... (checked below)."

Done.

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

https://chromiumcodereview.appspot.com/9290065/diff/1/runtime/vm/parser.h#new...
runtime/vm/parser.h:227: TypeResolution type_resolution);
On 2012/01/26 22:52:03, srdjan wrote:
> IMO, it looks better to have the result 'type' at the end of the argument
list.

Done.