Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary calls to Contains for sets #69179

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

mpidash
Copy link
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
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 23, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 23, 2023
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@mavasani
Copy link
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
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
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
Member

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
27 checks passed
@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