Issue 17586002: Resolve @Type.constructorName() and its arguments.

Issue 17586002: Resolve @Type.constructorName() and its arguments. (Closed)

Created:
7 years, 6 months ago by scheglov

Modified:
7 years, 6 months ago

Reviewers:
jwren, Brian Wilkerson

CC:
reviews_dartlang.org, lukeechurch_gmail.com

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

Visibility:
Public.

More Reviews

Description

Resolve @Type.constructorName() and its arguments. R=brianwilkerson@google.com, jwren@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=24375

Patch Set 2 : Resolve @A() - unnamed constructor invocation. #

Created: 7 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+142 lines, -10 lines)			Patch
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/Annotation.java	View	1	3 chunks	+19 lines, -3 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java	View	1	4 chunks	+59 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java	View	1	2 chunks	+4 lines, -1 line	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java	View	1	1 chunk	+13 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticWarningCodeTest.java	View	1	1 chunk	+26 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightings.java	View	1	2 chunks	+21 lines, -5 lines	0 comments	Download
M	tests/co19/co19-analyzer.status	View	1	1 chunk	+0 lines, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Brian Wilkerson

LGTM https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java (right): https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java#newcode926 editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:926: // nit: please move line 926 to line ...

7 years, 6 months ago (2013-06-24 13:57:48 UTC) #2

jwren

On 2013/06/24 13:57:48, Brian Wilkerson wrote: > LGTM > > https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java > File > editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java ...

7 years, 6 months ago (2013-06-24 16:44:55 UTC) #3

On 2013/06/24 13:57:48, Brian Wilkerson wrote:
> LGTM
> 
>
https://codereview.chromium.org/17586002/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/17586002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:926:
> //
> nit: please move line 926 to line 945
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:927:
> if (node.getParent() instanceof Annotation && prefixElement instanceof
> ClassElement) {
> I suspect that we need to do something similar in visitSimpleIdentifier to
> handle cases like "@SomeClass(...)". Please at least add a TODO comment so
that
> we don't forget to come back to it.
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
> File
>
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java
> (right):
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java:2027:
> "  A.name() {}",
> Doesn't this need to be a const constructor in order to be used in an
> annotation? (I don't have the spec available at the moment.) If so, then this
> indicates that we're not correctly checking 'const'ness in annotations. Please
> open an issue, create a failing test, or add a comment so that we don't forget
> to fix this.
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
> File
>
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticWarningCodeTest.java
> (right):
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticWarningCodeTest.java:265:
> "  A.fromInt(int p) {}",
> Again, I think this needs to be a const constructor.
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
> File
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightings.java
> (right):
> 
>
https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightings.java:89:
> ASTNode parent = node.getParent();
> nit: I'd move line 89 to just after line 93 so we don't access the parent
unless
> we need it.

Pending Brian's comments, LGTM

scheglov

PTAL I've added resolving @A() and TODOs. https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java (right): https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java#newcode926 editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:926: // On ...

7 years, 6 months ago (2013-06-24 21:24:54 UTC) #4

PTAL

I've added resolving @A() and TODOs.

https://codereview.chromium.org/17586002/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/17586002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:926:
//
On 2013/06/24 13:57:48, Brian Wilkerson wrote:
> nit: please move line 926 to line 945

Done.

https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:927:
if (node.getParent() instanceof Annotation && prefixElement instanceof
ClassElement) {
On 2013/06/24 13:57:48, Brian Wilkerson wrote:
> I suspect that we need to do something similar in visitSimpleIdentifier to
> handle cases like "@SomeClass(...)". Please at least add a TODO comment so
that
> we don't forget to come back to it.

Done.

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

https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java:2027:
"  A.name() {}",
On 2013/06/24 13:57:48, Brian Wilkerson wrote:
> Doesn't this need to be a const constructor in order to be used in an
> annotation? (I don't have the spec available at the moment.) If so, then this
> indicates that we're not correctly checking 'const'ness in annotations. Please
> open an issue, create a failing test, or add a comment so that we don't forget
> to fix this.

I've added TODO, will fix in one of the next CLs.

https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightings.java
(right):

https://codereview.chromium.org/17586002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightings.java:89:
ASTNode parent = node.getParent();
On 2013/06/24 13:57:48, Brian Wilkerson wrote:
> nit: I'd move line 89 to just after line 93 so we don't access the parent
unless
> we need it.

Done.

scheglov

7 years, 6 months ago (2013-06-24 23:27:00 UTC) #6

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages