Issue 10968010: Import scope rules updated.

Issue 10968010: Import scope rules updated. (Closed)

Created:
8 years, 3 months ago by Johnni Winther

Modified:
8 years, 3 months ago

Reviewers:
ahe, Lasse Reichstein Nielsen, ngeoffray

CC:
reviews_dartlang.org, karlklose, ngeoffray, kasperl, floitsch

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Import scope rules updated. BUG=2946 Committed: https://code.google.com/p/dart/source/detail?r=12706

Patch Set 2 : Updated cf. comments #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+72 lines, -19 lines)			Patch
M	lib/compiler/implementation/elements/elements.dart	View	1	4 chunks	+48 lines, -9 lines	0 comments	Download
M	lib/compiler/implementation/resolver.dart	View	1	4 chunks	+16 lines, -3 lines	0 comments	Download
M	lib/compiler/implementation/scanner/scanner_task.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	lib/compiler/implementation/ssa/builder.dart	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	lib/compiler/implementation/warnings.dart	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	tests/co19/co19-dart2dart.status	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	tests/co19/co19-dart2js.status	View	1	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Lasse Reichstein Nielsen

LGTM https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/elements/elements.dart File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/elements/elements.dart#newcode491 lib/compiler/implementation/elements/elements.dart:491: final Map<SourceString, Element> importScope; Documentation. What is the ...

8 years, 3 months ago (2012-09-20 12:21:08 UTC) #2

ngeoffray

LGTM https://codereview.chromium.org/10968010/diff/1/tests/language/library1a_test.dart File tests/language/library1a_test.dart (right): https://codereview.chromium.org/10968010/diff/1/tests/language/library1a_test.dart#newcode5 tests/language/library1a_test.dart:5: // This tests that it is a compile-time ...

8 years, 3 months ago (2012-09-20 12:21:58 UTC) #3

Lasse Reichstein Nielsen

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/elements/elements.dart File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/elements/elements.dart#newcode520 lib/compiler/implementation/elements/elements.dart:520: if (existing !== element && existing !== null) { ...

8 years, 3 months ago (2012-09-20 14:03:36 UTC) #4

Johnni Winther

8 years, 3 months ago (2012-09-21 14:00:05 UTC) #5

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/e...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:491: final Map<SourceString,
Element> importScope;
On 2012/09/20 12:21:08, Lasse Reichstein Nielsen wrote:
> Documentation. What is the meaning of this field? What is it used for?
> 
> Should this be a Scope instead of a Map? Or some other meaningful wrapping of
> the map, to avoid people *fiddling* with the Map from the outside?

Comment added.

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:512: * Adds [element] to the
imported scope of this library.
On 2012/09/20 12:21:08, Lasse Reichstein Nielsen wrote:
> "imported scope" -> "import scope". You don't import a scope. You import into
> the scope.

Done.

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:520: if (existing !== element
&& existing !== null) {
On 2012/09/20 12:21:08, Lasse Reichstein Nielsen wrote:
> Adding the same element twice is an error (according to current spec). It's
> importing the same name twice that is not allowed, independently of the actual
> name.
> 

I don't understand. Did you mean '... is not an error ...'? The created
[ErroneousElement] is used to report errors/warnings on usage.

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/r...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/10968010/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1044: = new
ResolutionWarning(erroneousElement.messageKind,
On 2012/09/20 12:21:08, Lasse Reichstein Nielsen wrote:
> Put the '=' on the previous line.

Done.

https://codereview.chromium.org/10968010/diff/1/tests/language/library1a_test...
File tests/language/library1a_test.dart (right):

https://codereview.chromium.org/10968010/diff/1/tests/language/library1a_test...
tests/language/library1a_test.dart:5: // This tests that it is a compile-time
error to both import a library
On 2012/09/20 12:21:58, ngeoffray wrote:
> Update comment.

Done.

Expand Messages | Collapse Messages