Skip to content

Annotate bound nodes and local lowering.#42068

Merged
gafter merged 8 commits intodotnet:masterfrom
gafter:annotate-local-lowering
Mar 17, 2020
Merged

Annotate bound nodes and local lowering.#42068
gafter merged 8 commits intodotnet:masterfrom
gafter:annotate-local-lowering

Conversation

@gafter
Copy link
Copy Markdown
Member

@gafter gafter commented Mar 2, 2020

No description provided.

@gafter gafter added this to the 16.6 milestone Mar 2, 2020
@gafter gafter requested a review from a team as a code owner March 2, 2020 01:02
@gafter gafter self-assigned this Mar 2, 2020
@jcouv jcouv added the Concept-Null Annotations The issue involves annotating an API for nullable reference types label Mar 2, 2020
@gafter
Copy link
Copy Markdown
Member Author

gafter commented Mar 2, 2020 •
edited
Loading

@dotnet/roslyn-compiler May I please have a couple of reviews of this? #Resolved

@jcouv jcouv self-assigned this Mar 2, 2020
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 2, 2020 •
edited
Loading

Looking
#Closed

jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/BoundNode_Source.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/BoundPattern_ConvertedType.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/BoundTree/Statement.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs Outdated
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs
jcouv
jcouv reviewed Mar 2, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs Outdated
333fred
333fred reviewed Mar 7, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_TryStatement.cs Outdated
/// that it may return true when the statement actually may have no side effects.
/// </summary>
private static bool HasSideEffects(BoundStatement statement)
private static bool HasSideEffects(BoundStatement? statement)
Copy link
Copy Markdown
Member

@333fred 333fred Mar 10, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

Consider annotated with NotNullWhen(true) #WontFix

Copy link
Copy Markdown
Member Author

@gafter gafter Mar 12, 2020

Choose a reason for hiding this comment

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

I could not find any client who would benefit from that.

In reply to: 390545649 [](ancestors = 390545649)

Copy link
Copy Markdown
Member

@333fred 333fred Mar 13, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

I could not find any client who would benefit from that.

The point is not about today's clients. Someone might benefit from this in the future, and the condition is already met by the implementation. #Resolved

333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_TupleBinaryOperator.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_TupleBinaryOperator.cs Outdated
333fred
333fred reviewed Mar 10, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Yield.cs
@333fred
Copy link
Copy Markdown
Member

333fred commented Mar 10, 2020 •
edited by gafter
Loading

Done review pass (commit 3) #Resolved

@gafter
Copy link
Copy Markdown
Member Author

gafter commented Mar 12, 2020 

        ConstantValue constantValueOpt,

This API is not annotated.

In reply to: 594907751 [](ancestors = 594907751)

Refers to: src/Compilers/CSharp/Portable/BoundTree/Constructors.cs:380 in 7d3fbf2. [](commit_id = 7d3fbf2, deletion_comment = False)

@gafter
Copy link
Copy Markdown
Member Author

gafter commented Mar 12, 2020 •
edited
Loading

@jcouv @333fred I think I've addressed all of your comments in the latest iteration. Do you have any other comments? #Resolved

jcouv
jcouv reviewed Mar 13, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ThrowStatement.cs Outdated

#nullable enable

using System.Diagnostics;
Copy link
Copy Markdown
Member

@jcouv jcouv Mar 13, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

nit: unnecessary using? #Resolved

jcouv
jcouv approved these changes Mar 13, 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 7)

333fred
333fred reviewed Mar 13, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Compiler/TypeCompilationState.cs Outdated
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs Outdated
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_TryStatement.cs Outdated
/// that it may return true when the statement actually may have no side effects.
/// </summary>
private static bool HasSideEffects(BoundStatement statement)
private static bool HasSideEffects(BoundStatement? statement)
Copy link
Copy Markdown
Member

@333fred 333fred Mar 13, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

I could not find any client who would benefit from that.

The point is not about today's clients. Someone might benefit from this in the future, and the condition is already met by the implementation. #Resolved

333fred
333fred reviewed Mar 13, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ThrowStatement.cs Outdated
@333fred
Copy link
Copy Markdown
Member

333fred commented Mar 16, 2020 •
edited by gafter
Loading

@gafter waiting for response to my latest comments. #Resolved

@gafter
Copy link
Copy Markdown
Member Author

gafter commented Mar 16, 2020

@333fred I think I've responded to your latest round of comments.

333fred
333fred approved these changes Mar 17, 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.

LGTM (commit 8)

@gafter gafter merged commit 6e804ae into dotnet:master Mar 17, 2020
@ghost ghost modified the milestones: 16.6, Next Mar 17, 2020
@sharwell sharwell modified the milestones: Next, 16.6.P2 Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@333fred 333fred

@gafter gafter

@jcouv jcouv

Labels

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

Projects

None yet

Milestone

16.6.P2

Development

Successfully merging this pull request may close these issues.

4 participants

@gafter @jcouv @333fred @sharwell