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

created destructor for LeakDetector #1063

Closed
wants to merge 1 commit into from

Conversation

bigboynaruto
Copy link

Description

This code generates memory leaks.

#include "catch.hpp"

int main(int argc, char **argv) {
    if (false) { // do not run tests
        return Catch::Session().run(argc, argv);
    }
}

GitHub Issues

@philsquared
Copy link
Collaborator

It does.
Is there a reason this is a problem?
One of Catch::Session's roles is to clean up singletons in it's destructor.
If you want to do it without Catch::Session for some reason you can explicitly call Catch::cleanUp() instead.

@philsquared
Copy link
Collaborator

philsquared commented Oct 23, 2017

-- sorry didn't notice before that your PR was to put Catch::cleanUp() in LeakDetectors destructor.
That won't work for two reasons:

  1. cleanup needs to have run before leak detection
  2. cleanup should only be called once (we could put a flag in, but IIRC that's not how it works now) - so if you do have a Session object too it will break.

@bigboynaruto
Copy link
Author

So is it ok If I create this flag?

@philsquared
Copy link
Collaborator

Actually, looking at the code it might work ok. It deletes objects then sets them to null. I think it used to be more problematic.
Also the leak detection happens later than the LeakDetector's destructor.
So maybe your PR is ok as-is

@bigboynaruto
Copy link
Author

Ok.

@horenmar
Copy link
Member

Is there a reason why you add it only in non-Windows LeakDetector?

@bigboynaruto
Copy link
Author

No, I just don't have Windows, so I can't test it.

@JoeyGrajciar
Copy link
Contributor

@bigboynaruto Just checked on Windows and adding it also for Windows LeakDetector would help :)

@horenmar
Copy link
Member

The linked PR actually added this properly, so I am going to close this.

@horenmar horenmar closed this Oct 18, 2018
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.

4 participants