Concept ACK: -Wswitch is good, and termination via assert(false); is better than UB :)

From the standard: "Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function." (C++11, [stmt.return])

Concept ACK

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: deprecate addresses and reqSigs from rpc outputs #20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

We do switch (dest.which()) { in a few places as well. This seems pretty similar, and I'm wondering if this should be refactored as well? (Probably in a different PR though?)

What do you suggest? which decays the enum class into an integer. The compiler can't do -Wswitch static analysis on those switch statements.

Well once we have (would require #20286):

bool ExtractDestination(const CScript& scriptPubKey, TxoutType& typeRet, CTxDestination& addressRet);

Could we do something like:

TxoutType type;
CTxDestination dest;
ExtractDestination(m_script, type, dest);
switch (type) {
case TxoutType::PUBKEYHASH:
case TxoutType::SCRIPTHASH: return OutputType::LEGACY;
case TxoutType::WITNESS_V0_SCRIPTHASH:
case TxoutType::WITNESS_V0_KEYHASH:
case TxoutType::WITNESS_V1_TAPROOT:
case TxoutType::WITNESS_UNKNOWN: return OutputType::BECH32;
case TxoutType::PUBKEY:
case TxoutType::MULTISIG:
case TxoutType::NONSTANDARD:
case TxoutType::NULL_DATA: return nullopt; // no default case, so the compiler can warn about missing cases
}
assert(false);

Note: I messed around with this, and probably made some mistakes here, but didn't see any unit tests or functional tests fail. So maybe some test coverage is needed here as well?

would require #20286

Let's discuss this in the other pull then

cr ACK fa650ca: patch looks correct and assert(false); is better than UB :)