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::terminate:

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 << syntax.

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: PrintingFn throws an exception, the stack is unwound, LogMessageFatalThrow of the calling scope gets destructed, which throws a second exception. As the warning suggests, this has undefined behavior - on Unix this causes an std::terminate.

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 std::uncaught_exception. I'll give it a try.

@ahojnnes @whuaegeanse Done, PTAL.