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.

Sign up for GitHub

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

Jump to bottom

Don't guard Dictionary<K, V>.Remove(key) by ContainsKey(key) #4836

Merged
merged 47 commits into from
Apr 29, 2022
Merged

Don't guard Dictionary<K, V>.Remove(key) by ContainsKey(key) #4836

merged 47 commits into from
Apr 29, 2022

Conversation

chucker
Copy link
Contributor

@chucker chucker commented Feb 11, 2021

This implements dotnet/runtime#33797, including a small number of tests for both C# and VB.

By design, the diagnostic will not be reported if the condition contains multiple statements, as suggested by @carlossanlop. That means it doesn't yet include @terrajobst's suggested further improvement of hoisting the Remove call into the condition, and leaving the remaining statements inside the block.

@chucker chucker requested a review from a team as a code owner February 11, 2021 23:09
@dnfadmin
Copy link

dnfadmin commented Feb 11, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@Youssef1313
Copy link
Member

You'll need to rebase on .NET 6 release branch.

@mavasani Should this go to preview 1 or preview 2 branches?

TIP: I'd suggest hard-resetting the last commit (13c184b) before rebasing. This should decrease conflicts, then re-run pack target after rebasing.

mavasani
mavasani previously requested changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

New .NET6.0 analyzers must target release/6.0.1xx-preview2 branch, please retarget.

@chucker chucker force-pushed the features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey branch from 13c184b to 0a3bdb3 Compare February 16, 2021 16:33
@chucker chucker changed the base branch from master to release/6.0.1xx-preview2 February 16, 2021 16:34
Evangelink
Evangelink reviewed Feb 17, 2021
View reviewed changes
...zers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs Outdated Show resolved Hide resolved
...zers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs Outdated Show resolved Hide resolved
...zers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs Outdated Show resolved Hide resolved
...zers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs Outdated Show resolved Hide resolved
...ests/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKeyTests.cs Show resolved Hide resolved
...ore/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.Fixer.cs Outdated Show resolved Hide resolved
@Evangelink
Copy link
Member

Thanks @chucker. You will need to apply the code-fix to add the new rule entry.

@chucker
Copy link
Contributor Author

chucker commented Feb 18, 2021

Thanks @chucker. You will need to apply the code-fix to add the new rule entry.

Not sure how I managed to write a code fix and not consider the possibility that adding this rule is as simple as invoking a code fix. :-)

@chucker chucker force-pushed the features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey branch from 5a836cb to 8155018 Compare February 18, 2021 22:20
@Evangelink Evangelink self-requested a review February 18, 2021 22:22
@buyaa-n
Copy link
Member

buyaa-n commented Mar 3, 2022

New .NET6.0 analyzers must target release/6.0.1xx-preview2 branch, please retarget.

@mavasani this PR is ready for review, need your approval as change requested

@mavasani mavasani dismissed their stale review April 1, 2022 17:01

Now targeting the correct branch

@buyaa-n buyaa-n requested a review from mavasani April 27, 2022 18:05
@mavasani
Copy link
Contributor

@buyaa-n Seems like there are real test failures.

@buyaa-n
Copy link
Member

buyaa-n commented Apr 28, 2022

@buyaa-n Seems like there are real test failures.

Failed on .net472 builds, looks Remove overload with out parameter is not exist in .Net framework, I will make that overload optional

@buyaa-n
Copy link
Member

buyaa-n commented Apr 29, 2022

Thanks @chucker for your contribution, the PR is finally ready for a merge.

Looks @carlossanlop requested a change long time ago to unify this PR with @CollinAlpert's PR, to ensure all 3 closely-related analyzers are handled together. But somehow @CollinAlpert were not able to push his changes to this PR, I had offline conversation with @carlossanlop about this issue and we decided to merge this PR first and let @CollinAlpert work on main branch instead. Seems Carlos forgot to remove the change request, as he is out in rest of the week I am dismissing it to unblock the PR

@buyaa-n buyaa-n dismissed carlossanlop’s stale review April 29, 2022 06:27

We decided to merge this PR first

@buyaa-n buyaa-n merged commit 757243c into dotnet:main Apr 29, 2022
@github-actions github-actions bot added this to the vNext milestone Apr 29, 2022
@CollinAlpert
Copy link
Contributor

Thanks @buyaa-n. So I will just create a new branch off of main and work on there?

@buyaa-n
Copy link
Member

buyaa-n commented May 1, 2022

Thanks @buyaa-n. So I will just create a new branch off of main and work on there?

Exactly

@CollinAlpert
Copy link
Contributor

I just merged main into my existing PR and made the necessary changes. It is ready to be reviewed.

@Youssef1313 Youssef1313 mentioned this pull request May 3, 2022
Closed
10 tasks
@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

@mavasani mavasani mavasani approved these changes

@Evangelink Evangelink Evangelink approved these changes

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

Assignees

@genlu genlu

@carlossanlop carlossanlop

@buyaa-n buyaa-n

Labels
None yet
Projects
None yet
Milestone
.NET vNext
Development

Successfully merging this pull request may close these issues.

None yet