Refactor Library/CompilationUnit and how we define local Scope.

Refactor Library/CompilationUnit and how we define local Scope.

This also collapses members and constructors of a class into the same
map.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=10549

[Lasse Reichstein Nielsen, 2012-08-08 14:22:12]

STV!

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:528: applyContainerPatch(original,
patches, original.findLocal);
If both ClassElement and LibraryElement define a findLocal method, there is no
need to pass it any more, you can just use original.findLocal directly.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:360: entryCompilationUnit =
new CompilationUnitElement(script, this);
In the longer run, maybe we should add entryCompilationUnit to compilationUnits.
For now, it's not what uses of compilationUnits expects.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:392: if (this ===
e.getLibrary()
The library test should be unnecessary when you only look at localMembers.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:395: if (!e.name.isPrivate())
f(e);
Put the !e.name.isPrivate() into the first if too.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:412: var path = uri.path;
var -> String.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:425: final LibraryElement
patchLibrary;
Why not store the CompilationUnitElement of patchLibrary.entryCompilationUnit
directly, instead of storing the library?

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:1002: // Search in scope to
be sure we search patched constructors.
Huh? Patched constructors are also members, as are injected constructors.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1878: //if
(element.constructors.length != 0) return;
Remove commented code.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/listener.dart:560: argumentName, prefix,
Too long, didn't read.
Perhaps extract library to a variable on a previous line.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/scanner_task.dart:134: ScriptTag tag,
[LibraryElement patch]) {
Again I'd prefer to have CompilationUnitElement here. That's all the prefix
element needs.

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/find_my_name_test.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/find_my_name_test.dart:20: for (Element e in
foo.localMembers) {
This is subtly equivalent to the previous version.
Make a comment that the implicit constructor is not in localMembers, even if it
is in localScope.

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/mock_compiler.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mock_compiler.dart:193: for (Link<Element> link =
imported.localMembers; !link.isEmpty();
Can you use for-in here too?

[Anders Johnsen, 2012-08-08 17:30:37]

PTAL

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:528: applyContainerPatch(original,
patches, original.findLocal);
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> If both ClassElement and LibraryElement define a findLocal method, there is no
> need to pass it any more, you can just use original.findLocal directly.

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:360: entryCompilationUnit =
new CompilationUnitElement(script, this);
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> In the longer run, maybe we should add entryCompilationUnit to
compilationUnits.
> For now, it's not what uses of compilationUnits expects.

I partly agree. It would be nice memory-wise, but those compilation units are
very different in meaning.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:392: if (this ===
e.getLibrary()
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> The library test should be unnecessary when you only look at localMembers.

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:395: if (!e.name.isPrivate())
f(e);
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Put the !e.name.isPrivate() into the first if too.

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:412: var path = uri.path;
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> var -> String.

While I believe we are moving away from this style, I'll do it :)

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:425: final LibraryElement
patchLibrary;
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Why not store the CompilationUnitElement of patchLibrary.entryCompilationUnit
> directly, instead of storing the library?

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:1002: // Search in scope to
be sure we search patched constructors.
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Huh? Patched constructors are also members, as are injected constructors.

Yes, but they are going to be hidden under a PatchedCompilationUnit, hiding the
properties of isConstructor(). We do, however, know that localScope is the
direct members that we expose from this class. Also, things as default
constructor implementations does now enter localMembers but only localScope.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1878: //if
(element.constructors.length != 0) return;
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Remove commented code.

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/listener.dart:560: argumentName, prefix,
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Too long, didn't read.
> Perhaps extract library to a variable on a previous line.

Done.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/scanner_task.dart:134: ScriptTag tag,
[LibraryElement patch]) {
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Again I'd prefer to have CompilationUnitElement here. That's all the prefix
> element needs.

I see. Will fix.

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/find_my_name_test.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/find_my_name_test.dart:20: for (Element e in
foo.localMembers) {
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> This is subtly equivalent to the previous version.
> Make a comment that the implicit constructor is not in localMembers, even if
it
> is in localScope.

A comment in this file, or in ClassElement?

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/mock_compiler.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mock_compiler.dart:193: for (Link<Element> link =
imported.localMembers; !link.isEmpty();
On 2012/08/08 14:22:12, Lasse Reichstein Nielsen wrote:
> Can you use for-in here too?

Done.

[ahe, 2012-08-08 17:39:05]

Very good. Unfortunately, you hit one of my idiosyncrasies :-)

I'm concerned about the implementation of the constructor getter. Not a biggie,
but I'd like to chat about it face2face tomorrow.

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:329: 
extra lines

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1017: return
localScope.getValues().filter((element) => element.isConstructor());
This scares me. Can't you run through the members?

[Lasse Reichstein Nielsen, 2012-08-10 11:32:17]

Still LGTM.

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1017: return
localScope.getValues().filter((element) => element.isConstructor());
I personally don't like the filter function.
In this case, I think it's ok. You run through all the local members, including
the synthetic constructor, and create a new collection of those of them that are
constructors.

To be more imperative, perhaps do:

List<Element> result = <Element>[];
localScope.forEach((_, Element value) => {
  if (value.isConstructor()) result.add(value);
});
return result;

[Anders Johnsen, 2012-08-13 06:22:41]

Committing.

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832203/diff/6002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1017: return
localScope.getValues().filter((element) => element.isConstructor());
On 2012/08/10 11:32:17, Lasse Reichstein Nielsen wrote:
> I personally don't like the filter function.
> In this case, I think it's ok. You run through all the local members,
including
> the synthetic constructor, and create a new collection of those of them that
are
> constructors.
> 
> To be more imperative, perhaps do:
> 
> List<Element> result = <Element>[];
> localScope.forEach((_, Element value) => {
>   if (value.isConstructor()) result.add(value);
> });
> return result;

I have no preference here, changing.