Fasta builders.

Fasta builders.

R=johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/ddee148c77505e90cd774b5195048110cebe3148

[ahe, 2017-01-13 12:22:59]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2017-01-16 13:01:19]

lgtm

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
File pkg/fasta/lib/src/builder/builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
pkg/fasta/lib/src/builder/builder.dart:87: int resolveTypes(Builder parent) =>
0;
Add dartdoc. Especially what the returned value means.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
pkg/fasta/lib/src/builder/builder.dart:93: Builder combineAmbiguousImport(String
name, Builder other,
Add dartdoc. Especially what role [library] has.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/c...
File pkg/fasta/lib/src/builder/class_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/c...
pkg/fasta/lib/src/builder/class_builder.dart:75: Builder
findStaticBuilder(String name, {bool isSetter: false}) {
Add dartdoc. When is this used? Is it only for getters/setters?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
File pkg/fasta/lib/src/builder/formal_parameter_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
pkg/fasta/lib/src/builder/formal_parameter_builder.dart:25: final bool hasThis;
Add dartdoc. I guess it is of the form `this.x`?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
File pkg/fasta/lib/src/builder/function_type_alias_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
pkg/fasta/lib/src/builder/function_type_alias_builder.dart:36: Scope scope =
library.scope.createNestedScope();
Only create the nested scope if `typeVariable != null`, otherwise use
`library.scope`.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/i...
File pkg/fasta/lib/src/builder/interface_type_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/i...
pkg/fasta/lib/src/builder/interface_type_builder.dart:17: // TODO(ahe): This
isn't really an interface type. Find better name.
NamedTypeBuilder? GenericTypeBuilder?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
File pkg/fasta/lib/src/builder/metadata_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/metadata_builder.dart:38: final String expression;
Why use String?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/metadata_builder.dart:40: final String postfix;
What is this?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
File pkg/fasta/lib/src/builder/mixed_accessor.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/mixed_accessor.dart:16: next = getter;
Do we know that `getter.next == setter` ?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
File pkg/fasta/lib/src/builder/prefix_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
pkg/fasta/lib/src/builder/prefix_builder.dart:35: return
builder.member.isInstanceMember ? null : builder.member;
How can an instance be returned from `exports[name]` ?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
pkg/fasta/lib/src/builder/prefix_builder.dart:47: Builder existing =
exports[name];
Why are we doing this for all names? Don't we risk handling the same name
multiple times?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dart
File pkg/fasta/lib/src/quote.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:77: }
Shouldn't we break after this? If we have

String c = '''abcde\t
''';

wouldn't we include 'abcde\t' in the prefix?

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:218: if (codeUnits.length == ++i) return error(i,
incompleteSequence);
Remove this and add a check before the loop, like for '\x..'

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:228: if (code > 0x10FFFF) return error(i,
invalidCodePoint);
Why check for '> 0xFFFF' first?

[ahe, 2017-01-16 15:26:34]

Thank you, Johnni!

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
File pkg/fasta/lib/src/builder/builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
pkg/fasta/lib/src/builder/builder.dart:87: int resolveTypes(Builder parent) =>
0;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Add dartdoc. Especially what the returned value means.

Done.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/b...
pkg/fasta/lib/src/builder/builder.dart:93: Builder combineAmbiguousImport(String
name, Builder other,
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Add dartdoc. Especially what role [library] has.

Done.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/c...
File pkg/fasta/lib/src/builder/class_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/c...
pkg/fasta/lib/src/builder/class_builder.dart:75: Builder
findStaticBuilder(String name, {bool isSetter: false}) {
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Add dartdoc. When is this used? Is it only for getters/setters?

Added:

  /// Lookup a static member of this class.

I also added this documentation to Builder.next:

  /// Used when multiple things with the same name are declared within the same
  /// parent. Only used for declarations, not for scopes.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
File pkg/fasta/lib/src/builder/formal_parameter_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
pkg/fasta/lib/src/builder/formal_parameter_builder.dart:25: final bool hasThis;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Add dartdoc. I guess it is of the form `this.x`?

Done and you guessed right.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
File pkg/fasta/lib/src/builder/function_type_alias_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/f...
pkg/fasta/lib/src/builder/function_type_alias_builder.dart:36: Scope scope =
library.scope.createNestedScope();
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Only create the nested scope if `typeVariable != null`, otherwise use
> `library.scope`.

I've added a TODO. It may work in this case, but for constructor initializers I
actually rely on knowing the exact number of enclosing scopes (although it makes
me sad).

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/i...
File pkg/fasta/lib/src/builder/interface_type_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/i...
pkg/fasta/lib/src/builder/interface_type_builder.dart:17: // TODO(ahe): This
isn't really an interface type. Find better name.
On 2017/01/16 13:01:19, Johnni Winther wrote:
> NamedTypeBuilder? GenericTypeBuilder?

I like NamedTypeBuilder. GenericTypeBuilder is a bit off as "member" may not be
generic (as in have type arguments), as it can be a type variable.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
File pkg/fasta/lib/src/builder/metadata_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/metadata_builder.dart:38: final String expression;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Why use String?

I generally use Strings to represent identifiers.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/metadata_builder.dart:40: final String postfix;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> What is this?

I've changed this to:

/// Expression metadata (without arguments).
///
/// Matches this grammar rule:
///
///    '@' qualified (‘.’ identifier)?
class ExpressionMetadataBuilder<T extends TypeBuilder>
    extends MetadataBuilder<T> {
  final String qualified;

  final String identifier;

  ExpressionMetadataBuilder(this.qualified, this.identifier);
}

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
File pkg/fasta/lib/src/builder/mixed_accessor.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/m...
pkg/fasta/lib/src/builder/mixed_accessor.dart:16: next = getter;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Do we know that `getter.next == setter` ?

No. Only things declared in the same parent can be found in next.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
File pkg/fasta/lib/src/builder/prefix_builder.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
pkg/fasta/lib/src/builder/prefix_builder.dart:35: return
builder.member.isInstanceMember ? null : builder.member;
On 2017/01/16 13:01:19, Johnni Winther wrote:
> How can an instance be returned from `exports[name]` ?

That shouldn't happen. I've turned it into an internal error.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/builder/p...
pkg/fasta/lib/src/builder/prefix_builder.dart:47: Builder existing =
exports[name];
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Why are we doing this for all names? Don't we risk handling the same name
> multiple times?

This deals with the case where the same prefix is used for different imports.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dart
File pkg/fasta/lib/src/quote.dart (right):

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:77: }
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Shouldn't we break after this? If we have
> 
> String c = '''abcde\t
> ''';
> 
> wouldn't we include 'abcde\t' in the prefix?

I think you're right. I copied this from
pkg/compiler/lib/src/string_validator.dart and it has the break.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:218: if (codeUnits.length == ++i) return error(i,
incompleteSequence);
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Remove this and add a check before the loop, like for '\x..'

Done.

https://codereview.chromium.org/2629063005/diff/1/pkg/fasta/lib/src/quote.dar...
pkg/fasta/lib/src/quote.dart:228: if (code > 0x10FFFF) return error(i,
invalidCodePoint);
On 2017/01/16 13:01:19, Johnni Winther wrote:
> Why check for '> 0xFFFF' first?

Probably because I was tired when I copied this from
pkg/compiler/lib/src/string_validator.dart :-)

[ahe, 2017-01-16 15:27:28]

Description was changed from

==========
Fasta builders.
==========

to

==========
Fasta builders.

R=johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ddee148c77505e90cd774b5195048110cebe3148
==========

[ahe, 2017-01-16 15:27:30]

Committed patchset #2 (id:20001) manually as
ddee148c77505e90cd774b5195048110cebe3148 (presubmit successful).