Issue 11361124: Introduce AmbiguousElement

Issue 11361124: Introduce AmbiguousElement (Closed)

Created:
8 years, 1 month ago by Johnni Winther

Modified:
8 years, 1 month ago

Reviewers:
ahe, karlklose

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Introduce AmbiguousElement Committed: https://code.google.com/p/dart/source/detail?r=14622

Patch Set 2 : Updated cf. comments. #

Total comments: 6

Patch Set 3 : Updated cf. comments. #

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+57 lines, -22 lines)			Patch
	M	sdk/lib/_internal/compiler/implementation/elements/elements.dart	View	1 2	4 chunks	+47 lines, -7 lines	0 comments	Download
	M	sdk/lib/_internal/compiler/implementation/resolution/members.dart	View	1 2	3 chunks	+10 lines, -15 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

ahe

Had you not used a cast... ;-) http://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/implementation/elements/elements.dart File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right): http://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/implementation/elements/elements.dart#newcode434 sdk/lib/_internal/compiler/implementation/elements/elements.dart:434: * ambiguous ...

8 years, 1 month ago (2012-11-06 16:28:10 UTC) #2

Johnni Winther

PTAL https://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/implementation/elements/elements.dart File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/implementation/elements/elements.dart#newcode434 sdk/lib/_internal/compiler/implementation/elements/elements.dart:434: * ambiguous element is encountered during resolution an ...

8 years, 1 month ago (2012-11-07 08:24:43 UTC) #3

ahe

LGTM, and it would look even better if AmbiguousElement always represented a pair of elements. ...

8 years, 1 month ago (2012-11-07 11:18:19 UTC) #4

karlklose

LGTM. https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler/implementation/elements/elements.dart File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler/implementation/elements/elements.dart#newcode453 sdk/lib/_internal/compiler/implementation/elements/elements.dart:453: AmbiguousElement(MessageKind this.messageKind, List this.messageArguments, Initializing formals get the ...

8 years, 1 month ago (2012-11-07 11:41:24 UTC) #5

Johnni Winther

8 years, 1 month ago (2012-11-07 13:02:01 UTC) #6

https://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11361124/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:451:
Link<Element> elements = const Link<Element>();
On 2012/11/07 11:18:19, ahe wrote:
> On 2012/11/07 08:24:43, Johnni Winther wrote:
> > On 2012/11/06 16:28:10, ahe wrote:
> > > Should this also be final? I would be more comfortable creating a new
> element
> > > rather than adding to an existing.
> > 
> > We need this to handle cases where more than two elements share the same
name.
> 
> In my mind that would be handled with:
> 
> new AbiguousElement(newElement, existingAmbiguousElement)

Done.

https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:453:
AmbiguousElement(MessageKind this.messageKind, List this.messageArguments,
On 2012/11/07 11:41:24, karlklose wrote:
> Initializing formals get the type from the field, so you can omit the type
> annotations here.

Done.

https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:714: this,
existing, element);
On 2012/11/07 11:41:24, karlklose wrote:
> Try fitting this and existing on the previous line.

Done.

https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11361124/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1272:
ambiguous.messageKind, ambiguous.messageArguments);
On 2012/11/07 11:41:24, karlklose wrote:
> I would prefer this formatting:
>   warnAndCreateErroneousElement(node, node.source, ambiguous.messageKind,  
>                                 ambiguous.messageArguments);

Done.

Expand Messages | Collapse Messages