Report CTEC.AMBIGUOUS_EXPORT

Report CTEC.AMBIGUOUS_EXPORT

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2013-05-10 13:19:22]

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:473:
checkForAmbiguousExport(node, exportElement);
Is there a reason why this can't be done in error verifier?

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java:67:
return Collections.unmodifiableMap(definedNames);
These changes are unnecessary. This is internal code, so (in my opinion) we
don't need to guard against misuse like we would if this were public API. And
given that we were returning a copy, what difference could it possibly make that
the copy can be modified?

[scheglov, 2013-05-10 14:42:27]

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:473:
checkForAmbiguousExport(node, exportElement);
On 2013/05/10 13:19:22, Brian Wilkerson wrote:
> Is there a reason why this can't be done in error verifier?

Initially there was a reason, but as the implementation is done now - no.
Fixed.

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java:67:
return Collections.unmodifiableMap(definedNames);
On 2013/05/10 13:19:22, Brian Wilkerson wrote:
> These changes are unnecessary. This is internal code, so (in my opinion) we
> don't need to guard against misuse like we would if this were public API. And
> given that we were returning a copy, what difference could it possibly make
that
> the copy can be modified?

Making copy costs something.
Creating unmodifiable view costs much less.

[Brian Wilkerson, 2013-05-10 14:56:04]

LGTM

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java:67:
return Collections.unmodifiableMap(definedNames);
> Making copy costs something.
> Creating unmodifiable view costs much less.

OK. But looking at the invocations of this method, none of them modify the
result, so perhaps we don't need either, just an addition to the Javadoc that
the result should not be modified.

https://codereview.chromium.org/14657012/diff/6001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/14657012/diff/6001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:465:
ExportElement exportElement = (ExportElement) element;
nit: perhaps we could just back out the changes to this file since they don't
add anything at this point.

[scheglov, 2013-05-10 15:13:54]

Committed patchset #2 manually as r22583 (presubmit successful).

[scheglov, 2013-05-10 15:14:44]

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java
(right):

https://codereview.chromium.org/14657012/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/Namespace.java:67:
return Collections.unmodifiableMap(definedNames);
On 2013/05/10 14:56:04, Brian Wilkerson wrote:
> > Making copy costs something.
> > Creating unmodifiable view costs much less.
> 
> OK. But looking at the invocations of this method, none of them modify the
> result, so perhaps we don't need either, just an addition to the Javadoc that
> the result should not be modified.

Rolled back change in this file.

https://codereview.chromium.org/14657012/diff/6001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/14657012/diff/6001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:465:
ExportElement exportElement = (ExportElement) element;
On 2013/05/10 14:56:04, Brian Wilkerson wrote:
> nit: perhaps we could just back out the changes to this file since they don't
> add anything at this point.

Done.