Optional disabling of exceptions

[chuim]

Our use of immer requires that we disable exceptions. As not supporting exceptions is a common situation in C++ codebases I'm filing this issue to propose integrating that change into immer.

Our modification follows the same pattern that LibC++ uses: if exceptions are enabled, use them, otherwise, if an out-of-range or an allocation fails, log a message and abort.

We added this snippet to immer/immer/config.hpp:

#if __has_feature(cxx_exceptions)
#define IMMER_THROW_BAD_ALLOC(msg) throw std::bad_alloc{msg}
#define IMMER_THROW_OUT_OF_RANGE(msg) throw std::out_of_range{msg}
#define IMMER_TRY try
#define IMMER_CATCH_ALL catch (...)
#define IMMER_RETHROW throw
#else
#define IMMER_NO_EXCEPTIONS
#define IMMER_THROW_BAD_ALLOC(msg) fprintf(stderr, "%s", msg); abort()
#define IMMER_THROW_OUT_OF_RANGE(msg) fprintf(stderr, "%s", msg); abort()
#define IMMER_TRY
#define IMMER_CATCH_ALL if (false)
#define IMMER_RETHROW
#endif

And then replaced all exception handling code with the appropriate macros.

Would it be OK to send a pull request with these modifications?

[arximboldi]

That sounds good!

I would however change it for:

#if __has_feature(cxx_exceptions)
#define IMMER_THROW(expr) throw expr
#define IMMER_TRY try
#define IMMER_CATCH(expr) catch (expr)
#define IMMER_RETHROW throw
#else
#define IMMER_NO_EXCEPTIONS
#define IMMER_THROW(expr) \
    do {                  \ 
        assert(! #expr);  \
        std::terminate(); \
    } while(false)
#define IMMER_TRY if (true)
#define IMMER_CATCH(expr) else
#define IMMER_RETHROW
#endif

Any objections against these macros instead?

[chuim]

Thanks for the reply. The only issue I see with using assert instead of a print function is that no error message would be printed on release builds (if NDEBUG is defined). But maybe that was your intention with this suggestion?

[yesudeep]

Do you think one could consider not simply aborting but returning some kind of "result" that allows the user to determine whether they want to use an exception, abort, or ignore it? Kind of like chrono does it.

[arximboldi]

@chuim Yes, I feel a bit uncomfortable in such a generic library forcing a particular form of output on the terminal, even in such exceptional conditions. std::terminate is the default technique in the standard to abort where exceptions do not apply, and sadly it does not take a message. If you really want to have some printing happen in that case, maybe we can do:

#ifndef IMMER_THROW(expr)
...
#endif

So you can customize that behavior at the build system level. Would that work for you?

@yesudeep Can you be more specific? I would not like to change the public interface of the library to support the "disable exceptions" case (also, most exceptions here are actually bad_alloc ones, which I believe should really not be treated with value-oriented errors, and to be honest, maybe it even is a mistake that the standard treats that with exceptions...)

[chuim]

@arximboldi Makes sense! I don't think we need a message after all. I'll send a PR as soon as I get some time to work on it, probably early next week.

[chuim]

I submitted pull request #162 but I've got a few comments/questions:

• I directly added the needed includes -- <cassert> and <exception> -- for the no-exceptions case of throw? Should I if-define them?
• Catch has no support for checking for program termination (related issue, no process isolation). For now I just disabled the catch expectations with if-define. I could try to override assert or std:terminate but I'm unsure that will bring any benefits.
• Style note: I kept the expected space between IMMER_CATCH and the parenthesis-wrapped exception name.

EDIT:

• I limited the change to the immer dir contents only.

[arximboldi]

I'm happy with the changes mostly, just:

Style note: I kept the expected space between IMMER_CATCH and the parenthesis-wrapped exception name.

The project uses clang-format, can you run it? Does it get confused with the macros?

[chuim]

I hadn't ran it before but now I did and I updated the pull request. It did change considerably how line breaks are treated around try/catch.

[arximboldi]

Yes... it looks funny. Do you know of any clang-format magic to convince it to treat the new macros like the old keywords? :/

[chuim]

I was able to improve it a bit with changes to .clang-format as seen here. The remaining oddity is the following, if we had the original non-MACRO code:

...
}  // This line break here!
catch (...) {
...

What do you think?

PS: Duplicating this comment here as I'm not really sure of what shows where. :)

[chuim]

With #162 merged, I believe this one can be considered resolved for now.