Skip to content

/ VisualStudio

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

Jump to bottom

Normalize Dispose implementations across the codebase #955

Closed
wants to merge 1 commit into from
Closed

Normalize Dispose implementations across the codebase #955

wants to merge 1 commit into from

Conversation

@shana
Copy link
Collaborator

@shana shana commented Apr 19, 2017

Went through our various Dispose() implementations and normalized them to the same pattern of:

bool disposed;
private [virtual/override] void Dispose(bool disposing)
{
  if (disposed) return;
  if (disposing)
  {
    disposed = true;
    // dispose managed stuff
  }
  // dispose native stuff
}

public void Dispose()
{
  Dispose(true);
  GC.SuppressFinalize(this);
}
@shana
Normalize Dispose implementations across the codebase
Loading status checks…
f3784e9
@shana shana requested review from grokys, jcansdale and paladique Apr 19, 2017
@grokys
Copy link
Contributor

@grokys grokys commented Apr 19, 2017

I notice some of the void Dispose(bool disposing) methods aren't virtual. Isn't the point of the dispose pattern to allow for derived classes to add their own dispose implementation? Without virtual there are no advantage of using the dispose pattern over just implementing Dispose().
@shana
Copy link
Collaborator Author

@shana shana commented Apr 19, 2017

@grokys yes indeed, good point. Also, I seem to have broken the credential cache tests with this one, so it might be that this pattern is still not completely correct, I'll take another crack at it.
@paladique paladique removed their request for review Jul 21, 2017
@shana shana force-pushed the master branch from 49ba6cf to a82bce9 Aug 16, 2017
@sguthals
Copy link
Contributor

@sguthals sguthals commented Mar 13, 2018

@shana - want to create an issue for this and re-visit when there is time to think about it?
@jcansdale jcansdale removed request for grokys and jcansdale Mar 10, 2020
@jcansdale jcansdale closed this Mar 10, 2020
@jcansdale jcansdale deleted the fixes/disposing branch Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@shana @grokys @sguthals @jcansdale
You can’t perform that action at this time.