arximboldi / immer

Postmodern immutable and persistent data structures for C++ — value semantics at scale
https://sinusoid.es/immer
Boost Software License 1.0
2.51k stars 185 forks source link

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

Open BillyDonahue opened 1 year ago

BillyDonahue commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sounds good.