Skip to content

Remove unnecessary calls to Contains for sets#69179

Merged
sharwell merged 3 commits intodotnet:mainfrom
mpidash:remove-guarded-set-calls
Jul 25, 2023
Merged

Remove unnecessary calls to Contains for sets#69179
sharwell merged 3 commits intodotnet:mainfrom
mpidash:remove-guarded-set-calls

Conversation

@mpidash
Copy link
Copy Markdown
Contributor

@mpidash mpidash commented Jul 23, 2023

This commit removes calls to Contains if they are immediately followed by either Add or Remove.
These calls were found during testing of a new analyzer (CA1865), see dotnet/roslyn-analyzers#6767 and dotnet/runtime#85490.

This commit removes calls to Contains if they are immediately followed
by either Add or Remove.
@mpidash mpidash requested review from a team as code owners July 23, 2023 21:49
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jul 23, 2023
@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 1)

@mavasani
Copy link
Copy Markdown
Contributor

This commit removes calls to Contains if they are immediately followed by either Add or Remove.
These calls were found during testing of a new analyzer (CA1865), see dotnet/roslyn-analyzers#6767 and dotnet/runtime#85490.

I believe you should also set CA1865 to be a build warning in the editorconfig at the repo root to prevent more violations sneaking in future.

@mpidash
Copy link
Copy Markdown
Contributor Author

mpidash commented Jul 24, 2023

I believe you should also set CA1865 to be a build warning in the editorconfig at the repo root to prevent more violations sneaking in future.

Should I add it for all [*.{cs,vb}]? That would be the first explicit rule there.
A few other rules are set for [src/{Analyzers,CodeStyle,Features,Workspaces,EditorFeatures,VisualStudio}/**/*.{cs,vb}].

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

Changes under Compilers LGTM (commit 2)

@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Jul 25, 2023

I believe you should also set CA1865 to be a build warning in the editorconfig at the repo root to prevent more violations sneaking in future.

It should be in .globalconfig, not .editorconfig.
https://github.com/dotnet/roslyn/blob/main/eng/config/globalconfigs/Common.globalconfig

@mpidash mpidash requested a review from a team as a code owner July 25, 2023 16:57
@sharwell sharwell merged commit d4bb429 into dotnet:main Jul 25, 2023
@ghost ghost added this to the Next milestone Jul 25, 2023
@mpidash mpidash deleted the remove-guarded-set-calls branch July 25, 2023 19:06
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants