Closed glebm closed 1 year ago
Hi,
Thanks for your PR. It would effectively be nice to support fno-exceptions, I didn't took the time to do it yet for this library.
Could you check out how it was done for the other tsl
projects (like in tsl::robin_map
) for example) and check if it can eventually be done the same way in the PR for coherence?
Thanks
I've pushed the following changes:
TSL_ENABLE_EXCEPTIONS
-> TSL_NO_EXCEPTIONS
.throw_
function to a macro.I think these are better in this PR as compared to the code in robin_map
:
std::abort
is used instead of std::terminate
, because std::terminate
is not available on some platform without exceptions.fprintf
instead of cerr
avoids the heavy iostreams
dependency.TSL_DEBUG
is defined.Thank you for the update. A few comments.
Could you use the SH
(sparse hash) like tsl_sh_assert
instead of `SM´?
std::abort is used instead of std::terminate, because std::terminate is not available on some platform without exceptions.
Yes, we can use std::abort
instead but could you then rename TSL_SM_THROW_OR_TERMINATE
to TSL_SH_THROW_OR_ABORT
fprintf instead of cerr avoids the heavy iostreams dependency.
Could you only print when TSL_DEBUG
is enabled? I would like to avoid importing cstdio
and printing to stderr by default.
I would prefer to avoid having an extra exceptions.h
file if possible, can't we have everything defined in sparse_growth_policy.h
? I know it is a bit confusing to add the functionality there compared to the name of the file but we need these to be defined before the exceptions usage in sparse_growth_policy.h
.
Is the CMake detection really useful compared to the detection in the header? Is there really a case where someone would want to use tsl::sparse_map
with exceptions enabled while they disable them for them?
Could you also add a config in the CI with exceptions disabled? Like https://github.com/Tessil/robin-map/blob/master/.github/workflows/ci.yml#L17
Thanks
I modified a bit your PR to address some of my comments, mainly moving things to sparse_growth_policy.h
to avoid a new file, renaming to TSL_SH_*
prefix and adapting the test to run an extra -fno-exceptions
CI. Hope it is alright for you.
I will merge the PR, thanks.
Thanks for following up on my PR!
Also allows controlling the use of exceptions on platforms with exceptions.
Fixes #18