Skip to content

Annotate implementations of ILabelSymbol#39197

Merged
sharwell merged 5 commits intodotnet:masterfrom
sharwell:symbol-annotations-12
Feb 3, 2020
Merged

Annotate implementations of ILabelSymbol#39197
sharwell merged 5 commits intodotnet:masterfrom
sharwell:symbol-annotations-12

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Oct 10, 2019

No description provided.

@sharwell sharwell requested a review from a team as a code owner October 10, 2019 06:20
@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 22, 2019
@sharwell sharwell added the Concept-Null Annotations The issue involves annotating an API for nullable reference types label Jan 3, 2020
@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jan 17, 2020

@dotnet/roslyn-compiler for review

@sharwell sharwell removed Blocked PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 17, 2020
jcouv
jcouv reviewed Jan 30, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/LabelSymbol.cs Outdated
jcouv
jcouv approved these changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 3)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jan 30, 2020

Tagging @333fred for a second review (small annotation PR)

@jcouv jcouv self-assigned this Jan 30, 2020
@333fred
Copy link
Copy Markdown
Member

333fred commented Jan 30, 2020 

    internal override TResult Accept<TArgument, TResult>(CSharpSymbolVisitor<TArgument, TResult> visitor, TArgument argument)

Should be annotated with MaybeNull on the return.

Refers to: src/Compilers/CSharp/Portable/Symbols/LabelSymbol.cs:123 in 05b44b8. [](commit_id = 05b44b8, deletion_comment = False)

@333fred
Copy link
Copy Markdown
Member

333fred commented Jan 30, 2020 

    public override TResult Accept<TResult>(CSharpSymbolVisitor<TResult> visitor)

Should be annotated with MaybeNull on the return.

Refers to: src/Compilers/CSharp/Portable/Symbols/LabelSymbol.cs:133 in 05b44b8. [](commit_id = 05b44b8, deletion_comment = False)

@333fred
Copy link
Copy Markdown
Member

333fred commented Jan 30, 2020

Done review pass (commit 3)

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jan 30, 2020

Should be annotated with MaybeNull on the return.

The C# symbol visitor is not yet annotated. These will be updated at that time.

333fred
333fred approved these changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew I was missing something.

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jan 30, 2020

@sharwell Looks ready to merge

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jan 30, 2020

@jcouv I'll remove the unnecessary using directive and allow a build to complete

sharwell
sharwell commented Jan 30, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/Source/SourceLabelSymbol.cs Outdated
if (tk.Kind() != SyntaxKind.None)
{
return tk.ValueText;
return tk.ValueText ?? "";
Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 This line is impacted by #41313

@sharwell sharwell force-pushed the symbol-annotations-12 branch from 80823ef to 8718b50 Compare February 3, 2020 17:03
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Feb 3, 2020

@sharwell Looks ready to merge

@sharwell sharwell merged commit 0bc1124 into dotnet:master Feb 3, 2020
@sharwell sharwell deleted the symbol-annotations-12 branch February 3, 2020 22:02
@sharwell sharwell added this to the 16.5.P3 milestone Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types

Projects

None yet

Milestone

16.5.P3

Development

Successfully merging this pull request may close these issues.

3 participants

@sharwell @jcouv @333fred