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

Fix FolderNameEditor memory leak on cancel #769

Merged
merged 4 commits into from Apr 16, 2019

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Apr 13, 2019

Fixes #768
Fixes #770
Fixes #767

@hughbe hughbe requested a review from a team as a code owner April 13, 2019 10:52
Copy link
Contributor

@weltkante weltkante left a comment

Choose a reason for hiding this comment

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

The double pointer on SHGetPathFromIDList looks suspicious.

@zsd4yr
Copy link
Member

zsd4yr commented Apr 15, 2019

@JeremyKuhne would you mind taking a look at this one as well?

@JeremyKuhne
Copy link
Member

Reviewing...

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this! Added a few more comments.

@codecov
Copy link

codecov bot commented Apr 15, 2019

Codecov Report

Merging #769 into master will increase coverage by 0.01048%.
The diff coverage is 25.86207%.

@@                 Coverage Diff                 @@
##              master        #769         +/-   ##
===================================================
+ Coverage   26.24093%   26.25141%   +0.01048%     
===================================================
  Files           1054        1057          +3     
  Lines         287909      287916          +7     
  Branches       38463       38461          -2     
===================================================
+ Hits           75550       75582         +32     
+ Misses        208380      208350         -30     
- Partials        3979        3984          +5
Flag Coverage Δ
#Debug 26.25141% <25.86207%> (+0.01048%) ⬆️
#production 17.56369% <3.05344%> (-0.00104%) ⬇️
#test 98.56541% <95.34884%> (-0.0045%) ⬇️

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks @hughbe!

}
finally
{
ArrayPool<char>.Shared.Return(displayName);
Copy link
Member

Choose a reason for hiding this comment

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

Just a note for future reference. In scenarios where perf is more critical and you don't expect errors to frequently happen it is ok to not return arrays to the pool (i.e. don't need to introduce a try block).

@zsd4yr
Copy link
Member

zsd4yr commented Apr 16, 2019

Thanks @hughbe and @JeremyKuhne !

@zsd4yr zsd4yr merged commit 547a013 into dotnet:master Apr 16, 2019
@hughbe hughbe deleted the foldernameeditor-tests branch April 16, 2019 18:41
@dotnet dotnet locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants