Fix warning C4722 #2391
Fix warning C4722 #2391
Conversation
|
Throwing from a destructor is indeed not a good idea and will leak memory. @sarlinpe FYI |
|
As mentioned in #2376, I checked with valgrind (on Unix) that this doesn't incur any memory leak. I agree that in general throwing from a destructor is bad design, but I think that it's fine for this case - unless the behavior is different for Unix vs Windows? The only scenario in which this could bite is the following, which causes an std::string PrintingFn(...) {
if (...) {
LOG(FATAL_THROW) << "Error in PrintingFn";
}
return ...;
}
LOG(FATAL_THROW) << "Error: " << PrintingFn(...);We don't have any such example in the codebase and it's easy to avoid. Besides, the throwing destructor is the only design (AFAIK) that allows the nice |
|
I assume that address sanitizer (which is already executed as part of CI) should also catch these. Maybe we should have a few unit tests that exercise the new exception macros explicitly, so they get executed in our CI pipeline. I am not sure I fully understand why the above example would cause a memory leak or std::terminate. I would have to dig into glog's code I guess? |
|
Having tests covering the macro is a good idea, I can give it a try unless @whuaegeanse you have time. Regarding the above example: |
|
OK, got it, thanks for the explanation. I guess we could detect such cases (where our log exceptions are thrown recursively) and then not throw a second time, crash the process, or log out both. This could be done with an atomic boolean flag or so in the LogFatalThrowLogMessage class. |
|
Not sure if there is another way to prevent the problem in the first place... Probably there is but requires time to dig. |
|
We can detect such cases by checking |
|
@ahojnnes @whuaegeanse Done, PTAL. |
Vistual Studio 2022 outputs compiler warning C4722.
warning C4722: 'colmap::LogMessageFatalThrow<std::invalid_argument>::~LogMessageFatalThrow<std::invalid_argument>': destructor never returns, potential memory leakThis PR attempts to suppress the C4722 warning. But I'm not sure if this is a good idea.
Compiler Warning (level 1) C4722