The compiler flag -Wswitch is enabled. However, it can not fire when a default: case exists. Fix that by removing the default case where a switch is already handling all cases exhaustively.
Also, enable -Wcovered-switch-default to catch those cases at compile time in the future.
Also, apply the comment according to the dev notes.
Can be reviewed via --ignore-all-space
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | purpleKarrot |
| Stale ACK | l0rinc, willcl-ark |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
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.
325+ assert(blocks_sig < dep.threshold);
326+ } else {
327+ assert(exp_state == ThresholdState::FAILED);
328+ }
329+ return;
330 }
0 } // no default case, so the compiler can warn about missing cases
ACK 6666ff4746d46d4946db94e6b5c94a2b450e4162
I had a similar attempt last week, had a few more cases covered (pun intended), but haven’t finished, so it’s likely not all correct - see if there’s anything you can include here: https://github.com/l0rinc/bitcoin/pull/119/changes
I had a similar attempt last week, had a few more cases covered (pun intended), but haven’t finished, so it’s likely not all correct - see if there’s anything you can include here: https://github.com/l0rinc/bitcoin/pull/119/changes
thx, taken. Except for the cases where the switch was not exhaustive. I think it is fine to have default cases that serve as a fallback, when the fallback is harmless. Also, I haven’t reviewed nor taken the GUI changes.
Excuse my almost-certainly-smooth-brained question, but, the PR title says “enable -Wswitch”, though that flag is not in the changeset.
Am I correct in inferring that, -Wall enables this by proxy, and then by removing the default case, this check is acually used/useful?
1124 throw JSONRPCError(RPC_MISC_ERROR, errors[0].original);
1125- break;
1126- }
1127- }
1128+ } // no default case, so the compiler can warn about missing cases
1129+ CHECK_NONFATAL(false);
Ah, no sorry, my first reply was wrong. The return type here is void, so the compiler can’t warn about a logic error here.
Though, I think it is still desirable to be warned about logic errors at runtime here.
For reference, the warning in another file would read:
0src/bench/sign_transaction.cpp:53:9: warning: control reaches end of non-void function [-Wreturn-type]
1 53 | }();
2 | ^
Also, about the gcc-only part, I think you are right according to https://godbolt.org/z/8M7rvEPKh. I guess clang tries to avoid what is a false-positive most of the time in real code (not going strictly after the C++ standard) See https://github.com/llvm/llvm-project/issues/33046#issuecomment-980982005
Though, that reminds me to enable -Wcovered-switch-default here.
Though, that reminds me to enable
-Wcovered-switch-defaulthere.
(done)
NONFATAL_UNREACHABLE and when assert(false), etc?
When are we doing that and when
NONFATAL_UNREACHABLEand whenassert(false), etc?
See the linter:
0test/lint/test_runner/src/lint_cpp.rs:CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
ACK fa6aa9e03be8fa5da38d82d39c79e652f910e1dd
This looks correct to me. I found other switch cases with default remaining, but none that qualify for a refactor-only PR, as they are non-exhaustive, non-enum, or othery types.
This appears to fix all exhaustive switches where removal of the default case is only a refactor.
The addition of the CHECK_NONFATAL in wallet/rpc/spend.cpp is needed to avoid -Wreturn-type error on gcc, as per https://gcc.gnu.org/wiki/VerboseDiagnostics#enum_switch (reading that, I wonder why clang doesn’t warn here too).
🚧 At least one of the CI tasks failed.
Task Windows native, VS: https://github.com/bitcoin/bitcoin/actions/runs/22524615324/job/65254621781
LLM reason (✨ experimental): test_bitcoin-qt failed due to an assertion failure in sendcoinsdialog.cpp (UI test assertion).
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
reACK fa84cd030f4c9bd03556c1a0e7b2206950002c0b
In addition to the second commit, I also checked this time that my version of gcc includes -Wswitch in -Wall, as I didn’t do that previously:
0[nix-shell:~/src/core/worktrees/pr-34684]$ gcc --version
1gcc (GCC) 15.2.0
2
3[nix-shell:~/src/core/worktrees/pr-34684]$ gcc -Wall -Q --help=warnings | grep enabled | grep switch
4 -Wswitch [enabled]
5 -Wswitch-bool [enabled]
6 -Wswitch-outside-range [enabled]
7 -Wswitch-unreachable [enabled]
The new second commit provides warnings if anyone tries to add a redundant default case to a covered switch, preventing regressions the other way. Notably this is a clang-only warning, not supported by gcc:
0[nix-shell:~/src/core/worktrees/pr-34684]$ gcc -Wcovered-switch-default -x c -c /dev/null -o /dev/null
1gcc: error: unrecognized command-line option ‘-Wcovered-switch-default’; did you mean ‘-Wno-switch-default’?
…but try_append_cxx_flags handles this correctly.
Wrapping RPCHelpMan in a lambda and returning makes more sense ergonomically.
872+ case Tabs::info: return 0;
873+ case Tabs::console: return 1;
874+ } // no default case, so the compiler can warn about missing cases
875+ assert(false);
876+ }();
877+ std::cout << ret << std::endl;
476@@ -477,9 +477,8 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
477 }
478 case TransactionStatus::NotAccepted:
479 return QIcon(":/icons/transaction_0");
480- default:
481- return COLOR_BLACK;
482- }
483+ } // no default case, so the compiler can warn about missing cases
484+ assert(false);
fa9379a refactor: Enable -Wswitch in exhaustive switch:
This is to avoid the Not all control paths return a value warning, right?
Wouldn’t NONFATAL_UNREACHABLE be a better fit here as a preparation for the C++23 std::unreachable()?
I don’t mind if we do that in a separate PR.
I don’t think std::unreachable makes sense for Bitcoin Core. std::unreachable is a way to invoke UB.
I think it is better to remove UB instead of adding more of it.
NONFATAL_UNREACHABLE is different from assert(false), because it is nonfatal, so I don’t think it makes sense here either.
89@@ -90,6 +90,9 @@ else()
90 try_append_cxx_flags("-Wconditional-uninitialized" TARGET nowarn_leveldb_interface SKIP_LINK
91 IF_CHECK_PASSED "-Wno-conditional-uninitialized"
92 )
93+ try_append_cxx_flags("-Wcovered-switch-default" TARGET nowarn_leveldb_interface SKIP_LINK
fa84cd0 build: Enable -Wcovered-switch-default:
I see that we’re checking one, but applying the opposite (here and above) - shouldn’t we check what we’re actually applying? Is it because probing the enabling form is stricter and the negative might just silently be accepted?
I think this is intentional, see the docs in the build system:
0CMakeLists.txt: # Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
1CMakeLists.txt: # set the -Wno-foo case if it works.
2CMakeLists.txt: IF_CHECK_PASSED "-Wno-unused-parameter"
1123+ case feebumper::Result::MISC_ERROR:
1124 throw JSONRPCError(RPC_MISC_ERROR, errors[0].original);
1125- break;
1126- }
1127- }
1128+ } // no default case, so the compiler can warn about missing cases
fa9379a refactor: Enable -Wswitch in exhaustive switch:
Could we fix https://github.com/bitcoin/bitcoin/blob/7072d825e39d200c5e49c736a281d3db180c716a/src/wallet/coinselection.cpp#L970 as well?
And https://github.com/bitcoin/bitcoin/blob/98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac/src/wallet/wallet.h#L286 seems to have a slightly different text
std::cout and have a more descriptive NONFATAL_UNREACHABLE instead of the hacky assert(false);
NACK
I consider “no default case, so the compiler can warn about missing cases” a compiler bug. Why can’t the compiler warn about missing cases when there is a default?
In my view, there should be two guidelines:
flag_enum) should cover all cases.With GCC, those two guidelines can be enforced with -Wswitch-default and -Wswitch-enum.
Clang does not support those and instead provides a -Wcovered-switch-default, which I consider harmful. It shows a lack of understanding how enums work in C and C++.
In this code:
0enum color: int { red, blue };
color is defined as a strong type with the same value range as int, with the two named constants red and blue. Repeat that: color has the same value range as int! A switch over red and blue is by no means “covered” or “exhausted”.
I consider “no default case, so the compiler can warn about missing cases” a compiler bug. Why can’t the compiler warn about missing cases when there is a default?
Hmmm, these are fair comments, and I think I find myself agreeing with them… Two things stand out to me:
We’re pretty much a gcc project (currently, for our release builds at least), and as noted gcc supports -Wswitch-enum which warns on missing cases even with a default: present. So the “can’t have both” tradeoff that motivates removing default: doesn’t actually apply to our release compiler.
The other concern is that without a default:, the runtime safety net we use by convention (assert after the switch) relies on the dev remembering to add it, or reviewers catching it being absent. Nothing warns if they forget. With -Wswitch-enum + -Wswitch-default, the default: (really ~ default: assert(false)) is structurally part of the switch and enforced by the compiler.
This PR’s clang warnings enforce a convention (no default: in exhaustive switches). GCC’s pair would enforce both exhaustiveness and runtime safety. That’s quite compelling to me.
Changing our developer-notes.md and all switches is going to be a much larger change though, and perhaps not worth it?
Also, apply the comment according to the dev notes.
Also, modify the dev notes to give a lambda-wrapped example.
Can be reviewed via --ignore-all-space
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
The leveldb exclusion is required to avoid this warning in the subtree:
```
src/leveldb/util/status.cc:63:7: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
63 | default:
| ^
1 warning generated.
```I consider “no default case, so the compiler can warn about missing cases” a compiler bug. Why can’t the compiler warn about missing cases when there is a default?
I don’t think this is about compilers, it is more about existing code using the default: as a catch for the remaining, unlisted cases. Enforcing -Wswitch-enum is certainly possible. It makes the code more verbose, which is probably the reason it hasn’t been done in the past.
Enabling -Wswitch-enum seems questionable, glancing at the output:
0./src/script/interpreter.cpp:930:29: warning: 107 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
1 930 | switch (opcode)
2 | ^~~~~~
3./src/script/interpreter.cpp:965:29: warning: 100 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
4 965 | switch (opcode)
5 | ^~~~~~
6./src/script/interpreter.cpp:484:21: warning: 26 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
7 484 | switch (opcode)
8 | ^~~~~~
My preference would be to discuss and enable that in a separate pull request, if there is need for it.
To clarify, there are two different patterns used in this codebase:
0bool IsApple(Fruit f) {
1 switch (f) {
2 case apple: return 1;
3 default: return 0; // all other fruits (or invalid values)
4 }
5}
and
0int Calories(Fruit f) {
1 switch (f) {
2 case apple: return 95;
3 case banana: return 105;
4 } // no default case, so the compiler can warn about missing cases
5 assert(false); // Any other value of Fruit would be a logic error or data corruption -> terminate
6}
Enabling -Wswitch-enum would disallow the first pattern.
Personally, I think it is perfectly fine (and desirable) to have both patterns at the disposal, and have a way to document which pattern is used, so that compilers and code readers can understand and enforce each pattern.