Skip to content

memento: Fix memory leak #12

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

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

jclsn
Copy link
Contributor

@jclsn jclsn commented Mar 12, 2025

The memento should be deleted after it is restored.

In a real example you would probably use std::shared_ptr for this, but I see the advantage of not using too many advanced data structures when teaching about patterns.

The memento should be deleted after it is restored.
@jclsn jclsn force-pushed the memento-fix-memory-leak branch from cc70925 to 5b98cdc Compare March 12, 2025 12:33
@neochief neochief merged commit 27e1176 into RefactoringGuru:main Mar 13, 2025
@neochief
Copy link
Contributor

Thanks!

@jclsn
Copy link
Contributor Author

jclsn commented Mar 13, 2025 via email

@neochief
Copy link
Contributor

@jclsn May I ask you for a favour? Can you check out this PR and see whether it's any good? #6

I'm not a C++ developer, so I can only rely on people like you to keep these examples decent.

@jclsn
Copy link
Contributor Author

jclsn commented Mar 14, 2025

I am still a C++ noob as well and am also a Linux user. I booted a Windows VM though to check. The only necessary change in that commit is the one for the Facade. GCC does automatically deduce the empty branch in the ternary, but MVSC does throw an error. The author of the commit himself said that he had aggressive warnings turned on. I don't think it should be necessary to fix all of those.

Other things I have noticed:

  • There is an #include<memory> missing in Strategy -> Does compile in MSVC as it automatically adds the misisng include, but not in GCC.
  • The ThreadSafe examples of Singleton have a memory leak as well, as the destructor is protected and there is no DeleteSingleton() method that could be called at the end of main.
  • Strategy is the only example using smart pointers, although this isn't used in any other example, which isn't consistent. Smart pointers are actually state of the art to handle memory in modern C++, but I think are out of the scope of the book. I tried a Udemy course on Design Patterns some years ago that used smart pointers and templates even, which I found quite overwhelming for a beginner.

@neochief
Copy link
Contributor

@jclsn Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants