Why does Clang/LLVM warn me about using default in a switch statement where all enumerated cases are covered?

Question

Consider the following enum and switch statement:

typedef enum {
    MaskValueUno,
    MaskValueDos
} testingMask;

void myFunction(testingMask theMask) {
    switch (theMask) {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
        default: {} //deal with an unexpected or uninitialized value
    }
};

I'm an Objective-C programmer, but I've written this in pure C for a wider audience.

Clang/LLVM 4.1 with -Weverything warns me at the default line:

Default label in switch which covers all enumeration values

Now, I can sort of see why this is there: in a perfect world, the only values entering in the argument theMask would be in the enum, so no default is necessary. But what if some hack comes along and throws an uninitialized int into my beautiful function? My function will be provided as a drop in library, and I have no control over what could go in there. Using default is a very neat way of handling this.

Why do the LLVM gods deem this behaviour unworthy of their infernal device? Should I be preceding this by an if statement to check the argument?

Answer 1

Here's a version that suffers from neither the problem clang's reporting or the one you're guarding against:

void myFunction(testingMask theMask) {
    assert(theMask == MaskValueUno || theMask == MaskValueDos);
    switch (theMask) {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
    }
}

Killian has explained already why clang emits the warning: if you extended the enum, you'd fall into the default case which probably isn't what you want. The correct thing to do is to remove the default case and get warnings for unhandled conditions.

Now you're concerned that someone could call your function with a value that's outside the enumeration. That sounds like failing to meet the function's prerequisite: it's documented to expect a value from the testingMask enumeration but the programmer has passed something else. So make that a programmer error using assert() (or NSCAssert() as you said you're using Objective-C). Make your program crash with a message explaining that the programmer is doing it wrong, if the programmer does it wrong.

Answer 2

Having a default label here is an indicator that you're confused about what you're expecting. Since you have exhausted all possible enum values explicitly, the default cannot possibly be executed, and you don't need it to guard against future changes either, because if you extended the enum, then the construct would already generate a warning.

So, the compiler notices that you have covered all bases but appear to be thinking that you haven't, and that is always a bad sign. By taking the minimal effort to change the switch to the expected form, you demonstrate to the compiler that what you appear to be doing is what you are actually doing, and you know it.

Answer 3

Clang is confused, having a default statement there is perfectly fine practice, it is known as defensive programming and is considered good programming practice (1). It is used plenty in mission-critical systems, though perhaps not in desktop programming.

The purpose of defensive programming is to catch unexpected errors that in theory would never happen. Such an unexpected error is not necessarily the programmer giving the function an incorrect input, or even an "evil hack". More likely, it could be caused by a corrupt variable: buffer overflows, stack overflow, runaway code and similar bugs not related to your function could be causing this. And in case of embedded systems, variables could possibly change because of EMI, particularly if you are using external RAM circuits.

As for what do write inside the default statement... if you suspect that the program has gone haywire once you ended up there, then you need some sort of error handling. In many cases you can probably just simply add an empty statement with a comment: "unexpected but doesn't matter" etc, to show that you have given thought to the unlikely situation.

---

(1) MISRA-C:2004 15.3.

Answer 4

Better still:

typedef enum {
    MaskValueUno,
    MaskValueDos,

    MaskValue_count
} testingMask;

void myFunction(testingMask theMask) {
    assert(theMask >= 0 && theMask<MaskValue_count);
    switch theMask {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
    }
};

This is less error-prone when adding items to the enum. You can skip the test for >= 0 if you make your enum values unsigned. This method only works if you have no gaps in your enum values, but that is often the case.

Answer 5

But what if some hack comes along and throws an uninitialized int into my beautiful function?

Then you get Undefined Behavior, and your default will be meaningless. There's nothing that you can possibly do to make this any better.

Let me be more clear. The instant someone passes an uninitialized int into your function, it is Undefined Behavior. Your function could solve the Halting Problem and it wouldn't matter. It is UB. There is nothing you can possibly do once UB has been invoked.

Answer 6

The default statement wouldn't necessarily help. If the switch is over an enum, any value that is not defined in the enum will end up executing undefined behaviour.

For all you know, the compiler can compile that switch (with the default) as:

if (theMask == MaskValueUno)
  // Execute something MaskValueUno code
else // theMask == MaskValueDos
  // Execute MaskValueDos code

Once you trigger undefined behaviour, there is no going back.

Answer 7

I also prefer to have a default: in all cases. I'm late to the party as usual, but... some other thoughts that I didn't see above:

• The particular warning (or error if also throwing -Werror) is coming from -Wcovered-switch-default (from -Weverything but not -Wall). If your moral flexibility allows you to turn off certain warnings (i.e., excise some things from -Wall or -Weverything), consider throwing -Wno-covered-switch-default (or -Wno-error=covered-switch-default when using -Werror), and in general -Wno-... for other warnings you find disagreeable.
• For gcc (and more generic behavior in clang), consult gcc manpage for -Wswitch, -Wswitch-enum, -Wswitch-default for (different) behavior in similar situations of enumerated types in switch statements.

• I also don't like this warning in concept, and I don't like its wording; to me, the words from the warning ("default label ... covers all ... values") suggest that the default: case will be always be executed, such as

  switch (foo) {
    case 1:
      do_something(); 
      //note the lack of break (etc.) here!
    default:
      do_default();
  }
  

  On first reading, this is what I thought you were running into -- that your default: case would always be executed because there's no break; or return; or similar. This concept is similar (to my ear) to other nanny-style (albeit occasionally helpful) commentary that spews forth from clang. If foo == 1, both functions will be executed; your code above has this behavior. I.e., fail to break-out only if you want to possibly keep executing code from subsequent cases! This appears, however, not to be your problem.

At the risk of being pedantic, some other thoughts for completeness:

• I do however think this behavior is (more) consistent with aggressive type checking in other languages or compilers. If, as you hypothesize, some reprobate does attempt to pass an int or something to this function, which is explicitly intending to consume your own specific type, your compiler should protect you equally well in that situation with an aggressive warning or error. BUT it doesn't! (That is, it seems that at least gcc and clang don't do enum type-checking, but I hear that icc does). Since you aren't getting type-safety, you could get value-safety as discussed above. Otherwise, as suggested in TFA, consider a struct or something that can provide type-safety.
• Another workaround could be creating a new "value" in your enum such as MaskValueIllegal, and not supporting that case in your switch. That would be eaten by the default: (in addition to any other wacky value)

Long live defensive coding!

Answer 8

Here's an alternative suggestion:
The OP is trying to protect against the case where someone passes in an int where an enum is expected. Or, more likely, where someone has linked an old library with a newer program using a newer header with more cases.

Why not change the switch to handle the int case? Adding a cast in front of the value in the switch eliminates the warning and even provides a degree of hint about why the default exists.

void myFunction(testingMask theMask) {
    int maskValue = int(theMask);
    switch(maskValue) {
        case MaskValueUno: {} // deal with it
        case MaskValueDos: {}// deal with it
        default: {} //deal with an unexpected or uninitialized value
    }
}

I find this much less objectionable than the assert() testing each of the possible values or even making the assumption that the range of enum values is well-ordered so that a simpler test works. This is just an ugly way of doing what default does precisely and beautifully.