-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Remove the need for C++ exceptions #156
base: develop
Are you sure you want to change the base?
Conversation
I guess my biggest question is why is there the need for Also you can't implement |
I assume you mean instead of just using Going back to the same example with exceptions: catch (const std::exception &e) {
std::string msg("path_discovery::verified_temporary_directories() saw exception thrown: ");
msg.append(e.what());
LLFIO_LOG_FATAL(nullptr, msg.c_str());
abort();
} The catch block actually uses the declared catch variable (in
I implemented LLFIO_THROW based on the libstdc++ docs, but only found actual use cases which just rethrow the caught exception (i.e. just use Using a function-like macro such as: #ifndef __cpp_exceptions
#define LLFIO_THROW(t)
#else
#define LLFIO_THROW(t) throw t
#endif would require more refactoring, as right now all throw statements look like this: LLFIO_THROW std::invalid_argument("Not a UTC deadline!"); so they would have to be refactored to something like this: LLFIO_THROW(std::invalid_argument("Not a UTC deadline!")); which I don't know if that's what you would want. |
How would people feel about something like https://godbolt.org/z/hvnW1hrGT ? |
Personally, I think this is an improvement - mainly it's much more readable compared to Something like this is what I mean. The only possible drawback I can immediately see compared to discarded lambdas is that the exception can't be passed by non-const reference. Of course, I don't see why anyone would catch by non-const reference so I don't think matters in practice at all. |
This PR removes the need for exceptions to be enabled in llfio in a fairly basic way, and thus allows for
-fno-exceptions
builds. (This fixes #127). llfio's behavior should remain unchanged with this PR when exceptions are enabled.However, with this PR, when c++ exceptions are disabled:
NoValuePolicy
for llfio'sbasic_result<T, E, NoValuePolicy>
type alias becomespolicy::terminate
. Thus, all wideerror()
calls terminate on failure instead of throwing exceptions. (policy::terminate
was chosen so that llfio mimics the standard library behavior when exceptions are disabled)error_info::throw_exception()
just aborts.error_from_exception(std::exception_ptr&&, std::error_code)
just returns an emptyerror_info
(although it shouldn't actually ever be called).There is a caveat, which is that with this solution, catch blocks which capture a specific exception must now be passed by value. This is because of how the new catch macro now declares the would-be caught exception as a variable so that the catch blocks still compile with exceptions disabled (even though they will never execute).
To illustrate this, this example with exceptions enabled:
turns into this with exceptions disabled:
If
e
was caught by const reference (i.e. declared asconst std::exception&
), this wouldn't compile when exceptions are disabled, as a const reference variable would be declared without an initializer.So far, this only occurs in two instances, both in
include/llfio/v2.0/detail/impl/path_discovery.ipp
. While there is surely a better solution, this may be an acceptable drawback (although having to remember this for any future specific try/catch blocks will probably become annoying).On my local (linux) system, all tests within ctest pass except
llfio_sl--utils
, but I cannot determine if its failure is actually related to this PR or only related specifically to my local system. My own personal basic use-case test executable was successful and has the same behavior with or without this PR.