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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
• #31507 (build: Use clang-cl to build on Windows natively by hebasto)
• #28802 (ArgsManager: support command-specific options by ajtowns)

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.

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.

ACK fa6aa9e

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?

Sorry, I forgot to write the motivation in the description. Fixed now

🚧 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).}

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:

1. All switch statements should have a default.
2. Switch statements over enums (unless they are marked as 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:

enum 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?

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:

./src/script/interpreter.cpp:930:29: warning: 107 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
  930 |                     switch (opcode)
      |                             ^~~~~~
./src/script/interpreter.cpp:965:29: warning: 100 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
  965 |                     switch (opcode)
      |                             ^~~~~~
./src/script/interpreter.cpp:484:21: warning: 26 enumeration values not explicitly handled in switch: 'OP_0', 'OP_PUSHDATA1', 'OP_PUSHDATA2'... [-Wswitch-enum]
  484 |             switch (opcode)
      |                     ^~~~~~

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:

bool IsApple(Fruit f) {
 switch (f) {
  case apple:  return 1;
  default: return 0; // all other fruits (or invalid values)
 }
}

and

int Calories(Fruit f) {
 switch (f) {
  case apple:  return 95;
  case banana: return 105;
 } // no default case, so the compiler can warn about missing cases
 assert(false); // Any other value of Fruit would be a logic error or data corruption -> terminate
}

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.

Edit: about enum .. {..} __attribute__((flag_enum));: I think enforcing a default for those could make sense. It can even be part of -Wswitch, but I think no compiler has this implemented yet.

Maybe #ifndef them so the default is omitted only on debug builds?

Maybe #ifndef them so the default is omitted only on debug builds?

Not sure what the benefit would be. Also, the net benefit is already possible today (and even enforced for RPC code):

Example from above:

int Calories(Fruit f) {
 switch (f) {
  case apple:  return 95;
  case banana: return 105;
 } // no default case, so the compiler can warn about missing cases
 assert(false); // Any other value of Fruit would be a logic error or data corruption -> terminate
}

Example for a release-only fallback:

int Calories(Fruit f) {
 switch (f) {
  case apple:  return 95;
  case banana: return 105;
 } // no default case, so the compiler can warn about missing cases
 Assume(false); // Any other value of Fruit would be a logic error or data corruption
 return DEFAULT_CALORIES;
}

Example for an RPC-style non-fatal check:

int Calories(Fruit f) {
 switch (f) {
  case apple:  return 95;
  case banana: return 105;
 } // no default case, so the compiler can warn about missing cases
 CHECK_NONFATAL(false); // Any other value of Fruit would be a logic error or data corruption -> throw
}

re-ACK fa4ec13, no changes except for addressing silent merge conflict from d339884