Use exception handling macros to allow building with exceptions disabled#202
Use exception handling macros to allow building with exceptions disabled#202Cry-Luca wants to merge 1 commit into
Conversation
| } else if (!response->body.empty() && response->body[0] == '{') // have to be JSON | ||
| { | ||
| try { | ||
| _TRY_BEGIN |
There was a problem hiding this comment.
where _TRY_BEGIN , _CATCH and _CATCH_END macroses are coming from?
There was a problem hiding this comment.
I had some time to look at Linux.
As prospero-clang had it defined as well, I hoped it was more or less standardized.
The best approach is to probably use custom macros instead. Would that work for you?
#if _HAS_EXCEPTIONS || defined(__EXCEPTIONS)
#define NAKAMA_TRY_BEGIN try {
#define NAKAMA_CATCH(e) } catch (e) {
#else
#define NAKAMA_TRY_BEGIN {
#define NAKAMA_CATCH(e) } if constexpr (false) {
#endif
#define NAKAMA_CATCH_END }There was a problem hiding this comment.
I think exception detection needs to be more that that. Example from Google's abseil lib: https://github.com/abseil/abseil-cpp/blob/ae7be71bd8f3de9e13956e37865c578c1c2da36b/absl/base/config.h#L331-L348
Having said that, proper noexcept support is a much bigger change that try/catch syntax and I am not sure it can be done safely. Few thoughts why:
Dependency throws
If dependency throws (like libprotobuf's json funs can do for example), then whole app gets std::terminate immediately with no recourse.
Even if we update build system, to compile dependencies with exceptions disabled, they internally will be calling abort() where they'd normally throw, which leads to the same outcome. It feels like major limitation and it can't be resolved without dropping all C++ dependencies.
Async functions
The *Async APIs need to be conditional on SDK-level exception support, because they rely on std::future, std::promise::set_exception, and std::make_exception_ptr.
These public API functions cannot be disabled with ad-hoc exception-detection #ifdefs around the declarations. If the SDK and the consuming application are built with different exception flags, the virtual interface may have different shapes in different translation units, changing the vtable layout and causing hard-to-diagnose runtime failures. With a symbol-based API this would likely become an obvious linker error, but the current ABI is not symbol-based.
Exception support should therefore be recorded as a first-class SDK build setting in config.h, and that setting should consistently control whether the *Async APIs are exposed.
Dependencies
If dependencies are left built as they are today, independently of the SDK's exception mode, the SDK may be compiled with exceptions disabled while a dependency is still compiled with exceptions enabled. If such a dependency throws and the exception escapes into SDK code, the result is fatal. Since the SDK frames were compiled without exception support, the crash may appear to originate from the SDK call site rather than from the dependency code that threw, making diagnosis harder.
To improve this we should align its exception mode with the SDK. Prefer a dependency-provided CMake option if one exists; otherwise pass the compiler-specific no-exceptions flag so that deps can take their no-exception code paths. This usually turns throw-based fatal paths into dependency-local abort() which will help with diagnostics, as abort will be attributed to that dependency frame.
CMake options
I think currently we unconditionally enable exceptions on MSVC, this needs to change.
Overall it is quite a big change and I am sympathetic to have first class support for no exceptions mode, but it needs to be done properly and test for all platforms.
|
@redbaron I totally understand your points. But having proper no exception support that can still guarantee safety was well beyond the scope of my change. I am closing the PR. |
Originally part of #197
@redbaron you mentioned it's missing definitions for macros like
_TRY_BEGIN. You mean on specific platforms?That could very well be the case, I have the ability to test only some of the platforms.