Optimize parsing of qualified identifiers.

Optimize parsing of qualified identifiers.
Committed: https://code.google.com/p/dart/source/detail?r=3509

[regis, 2012-01-21 00:00:12]

This speeds up the command below by about 11%

Release_ia32/dart_no_snapshot --compile_all --compiler_stats
../tests/language/src/HelloDartTest.dart

[siva, 2012-01-23 18:11:36]

lgtm

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

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5058: if (ResolveIdentInLocalScope(token_index_, ident,
&var_or_field)) {
Since we now take NULL as a param maybe this can be NULL

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6473:
ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
The CheckInstance and ASSERT could also be inside the
conditional (if (node != NULL))?

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6663: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
Should we be reporting this as an error?

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7146: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
In the cases where current token is "." and the qualifying token is not a
library prefix we end up calling ResolveIdentInLocalScope twice.

[siva, 2012-01-23 18:11:38]

lgtm

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

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5058: if (ResolveIdentInLocalScope(token_index_, ident,
&var_or_field)) {
Since we now take NULL as a param maybe this can be NULL

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6473:
ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
The CheckInstance and ASSERT could also be inside the
conditional (if (node != NULL))?

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6663: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
Should we be reporting this as an error?

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7146: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
In the cases where current token is "." and the qualifying token is not a
library prefix we end up calling ResolveIdentInLocalScope twice.

[hausner, 2012-01-23 18:31:24]

LGTM, but this code is very delicate. We might want to make sure we have plenty
of pathological code samples with ugly shadowing and misspellt type names to
make sure the compiler is solid.

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

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3954: if (CurrentToken() == Token::kLT &&
!TryParseTypeParameter()) {
I've been conditioned to expect extra parens here.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6663: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
The parens around type_name.ident seem weird.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7144: ASSERT(!is_top_level_);
Not sure whether you can assert this here. What if we parse code that contains a
"new List()" statement in the initializer of a top-level variable? While that is
not valid Dart code, it should not crash the compiler.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7399: *(qual_ident.ident),
The parens are unnecessary.

[regis, 2012-01-23 19:56:23]

Thank you both.

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

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3954: if (CurrentToken() == Token::kLT &&
!TryParseTypeParameter()) {
On 2012/01/23 18:31:24, hausner wrote:
> I've been conditioned to expect extra parens here.

Done.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5058: if (ResolveIdentInLocalScope(token_index_, ident,
&var_or_field)) {
On 2012/01/23 18:11:38, asiva wrote:
> Since we now take NULL as a param maybe this can be NULL

Done.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6473:
ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
On 2012/01/23 18:11:38, asiva wrote:
> The CheckInstance and ASSERT could also be inside the
> conditional (if (node != NULL))?

Done.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6479:
ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
Done here too.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6663: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
On 2012/01/23 18:31:24, hausner wrote:
> The parens around type_name.ident seem weird.

They were sometimes added, sometimes not. I now removed them at every location I
could find them.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6663: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
On 2012/01/23 18:11:38, asiva wrote:
> Should we be reporting this as an error?

Yes, we have to. The fact that the qualident is declared in the local scope
indicates that it is not a type as expected.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7144: ASSERT(!is_top_level_);
On 2012/01/23 18:31:24, hausner wrote:
> Not sure whether you can assert this here. What if we parse code that contains
a
> "new List()" statement in the initializer of a top-level variable? While that
is
> not valid Dart code, it should not crash the compiler.

As discussed, we already have top level initializers using const parsed by this
code. ParseNewOperator() is only called from ParsePrimary(), in which I added an
ASSERT(!is_top_level_);

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7146: ResolveIdentInLocalScope(type_pos,
*(type_name.ident), NULL)) {
On 2012/01/23 18:11:38, asiva wrote:
> In the cases where current token is "." and the qualifying token is not a
> library prefix we end up calling ResolveIdentInLocalScope twice.

Consider this example calling a named constructor:

  var Test;
  var i = new Test.named(10);  // This should be an error.

ParseQualIdent will stop parsing at the period, because Test is a local ident,
but not a prefix. Although we see a period, we have to repeat the test to report
the error.

To make the code more efficient and robust, we would need to   remember one of 3
possible states: unchecked, local, non-local. After submitting this cl, I'll
measure if it is worth it or not.

https://chromiumcodereview.appspot.com/9271023/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7399: *(qual_ident.ident),
On 2012/01/23 18:31:24, hausner wrote:
> The parens are unnecessary.

Done.