Skip to content

Simplify nested lambda#61745

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:simplify-static
Jun 7, 2022
Merged

Simplify nested lambda#61745
sharwell merged 1 commit intodotnet:mainfrom
sharwell:simplify-static

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 7, 2022

The isSubsumed local function is marked static, so the compiler will cache the delegate.

This was not flagged by our analyzer due to the following condition. I don't have a good answer here; the static modifier is great for clarity but bad for readability.

roslyn/src/Analyzers/CSharp/Analyzers/RemoveUnnecessaryLambdaExpression/CSharpRemoveUnnecessaryLambdaExpressionDiagnosticAnalyzer.cs

Lines 77 to 81 in 5ada1d5

// Don't simplify static lambdas. The user made them explicitly static to make it clear it must only cause
// a single allocation for the cached delegate. If we get rid of the lambda (and thus the static-keyword) it
// won't be clear anymore if the member-group-conversion allocation is cached or not.
if (anonymousFunction.Modifiers.Any(SyntaxKind.StaticKeyword))
return;

@sharwell sharwell requested a review from a team as a code owner June 7, 2022 15:49
@ghost ghost added the Area-Compilers label Jun 7, 2022
jcouv
jcouv approved these changes Jun 7, 2022
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 1)

@jcouv jcouv self-assigned this Jun 7, 2022
@sharwell sharwell mentioned this pull request Jun 7, 2022
Merged
@sharwell sharwell enabled auto-merge June 7, 2022 17:04
@sharwell sharwell merged commit 77f1b7a into dotnet:main Jun 7, 2022
@ghost ghost added this to the Next milestone Jun 7, 2022
@sharwell sharwell deleted the simplify-static branch June 8, 2022 16:31
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
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

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

4 participants

@sharwell @cston @jcouv @RikkiGibson