sparse-map icon indicating copy to clipboard operation
sparse-map copied to clipboard

Add support for platforms without exceptions

Open glebm opened this issue 1 year ago • 3 comments

Also allows controlling the use of exceptions on platforms with exceptions.

Fixes #18

glebm avatar Aug 11 '22 05:08 glebm

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

Tessil avatar Aug 11 '22 22:08 Tessil

I've pushed the following changes:

  1. TSL_ENABLE_EXCEPTIONS -> TSL_NO_EXCEPTIONS.
  2. Detection of the default even without CMake.
  3. Changed the throw_ function to a macro.

I think these are better in this PR as compared to the code in robin_map:

  1. std::abort is used instead of std::terminate, because std::terminate is not available on some platform without exceptions.
  2. fprintf instead of cerr avoids the heavy iostreams dependency.
  3. Always prints the error message before aborting. The other version does so only if TSL_DEBUG is defined.

glebm avatar Aug 12 '22 06:08 glebm

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

Tessil avatar Aug 14 '22 09:08 Tessil

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.

Tessil avatar Jan 15 '23 16:01 Tessil

Thanks for following up on my PR!

glebm avatar Jan 15 '23 20:01 glebm