Skip to content
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

IMMER_RETHROW causes -Wreturn-type warnings in IMMER_NO_EXCEPTIONS builds #264

Open
BillyDonahue opened this issue Jun 1, 2023 · 2 comments

Comments

@BillyDonahue
Copy link

BillyDonahue commented Jun 1, 2023

IMMER_RETHROW causes warnings in IMMER_NO_EXCEPTIONS builds.

Our build actually has exceptions support, but it's under GCC so immer thinks they're turned off. The detection of whether exceptions are supported or not is incorrect but I think that's a different bug (probably related to #168). This bug would apply equally to cases where exceptions were disabled deliberately.

Under GCC, with an unoptimized build, and with address sanitizer, this section of immer/detail/hamts/champ.hpp:

( https://github.com/arximboldi/immer/blob/master/immer/detail/hamts/champ.hpp#LL601C1-L614C1 )

                else {
                    auto child = node_t::make_merged(
                        shift + B, std::move(v), hash, *val, Hash{}(*val));
                    IMMER_TRY {
                        return {node_t::copy_inner_replace_merged(
                                    node, bit, offset, child),
                                true};
                    }
                    IMMER_CATCH (...) {
                        node_t::delete_deep_shift(child, shift + B);
                        IMMER_RETHROW;
                    }
                }

Yields a warning, which our build considers fatal. With ASAN, the compiler cannot see (for whatever reason) that the IMMER_TRY => if (true) is certain. So it considers IMMER_RETHROW to be reachable from the IMMER_CATCH => else branch.

The root cause is that IMMER_RETHROW is empty, (introduced by #160) and this looks like do_add falling off the end of the function without a return statement. I think IMMER_RETHROW should probably be a call to std::terminate or another noreturn function, so the compiler knows that it's not a return path.

src/third_party/immer/dist/immer/detail/hamts/champ.hpp:620:5: warning: control reaches end of non-void function [-Wreturn-type]
  620 |     }
      |     ^
@arximboldi
Copy link
Owner

Instead of std::terminate maybe it could become IMMER_UNREACHABLE which may further help get the compiler in the right direction.

I would be happy to accept fixes for both usses though: the exception suppport detection and this.

@BillyDonahue
Copy link
Author

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants