Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
1  
I should say, my reason for -Weverything is to make myself a better programmer. As the NSHipster says: "Pro tip: Try setting the -Weverything flag and checking the “Treat Warnings as Errors” box your build settings. This turns on Hard Mode in Xcode.". –  Swizzlr Dec 13 '12 at 12:36
1  
-Weverything can be useful, but be careful about mutating your code too much to deal with it. Some of those warnings are not only worthless but counter-productive, and are best turned off. (Indeed, that's the use case for -Weverything: start with it on, and turn off what doesn't make sense.) –  Steven Fisher Dec 13 '12 at 17:50
    
Could you expand a bit on the warning message for posterity? There's usually more to them than that. –  Steven Fisher Dec 13 '12 at 17:55
    
After reading the answers, I still prefer your initial solution. Usage of default statement IMHO is better than the alternatives given in the answers. Just a small note: the answers are really good and informative, they show a cool way how to solve the problem. –  bbaja42 Dec 13 '12 at 17:56
    
@StevenFisher That was the entire warning. And as Killian pointed out if I modify my enum later it would open the possibility for valid values to pass to the default implementation. It seems to be a good design pattern (if it is such a thing). –  Swizzlr Dec 13 '12 at 19:07

8 Answers 8

up vote 19 down vote accepted

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.

share|improve this answer
5  
+1. As a small modification, I prefer to have each case return; and add an assert(false); to the end (instead of repeating myself by listing the legal enums in an initial assert() and in the switch). –  Josh Kelley Dec 13 '12 at 15:08
    
What if the incorrect enum isn't passed by a dumb programmer, but rather by a memory corruption bug? When the driver presses the break of their Toyota and a bug has corrupted the enum, then the break-handling program should crash & burn, and there should be a text displayed to the driver: "the programmer is doing it wrong, bad programmer!". I don't quite see how this will help the user, before they drive over the edge of a cliff. –  user29079 Dec 13 '12 at 15:28
2  
@Lundin is that what the OP is doing, or is that a pointless theoretical edge case you've just constructed? Regardless, a "memory corruption bug" [i] is programmer error, [ii] is not something you can continue from in a meaningful way (at least not with the requirements as stated in the question). –  user4051 Dec 13 '12 at 15:33
    
*is probably programmer error. Unless the computer is in a satellite (in which case it isn't under a Toyota, obviously) programmer error is more likely to corrupt a value on the stack than a cosmic ray. Probably even in the satellite case, in fact. –  user4051 Dec 13 '12 at 15:44
    
@GrahamLee I'm just saying that "lay down and die" isn't necessarily the best option when you spot an unexpected error. –  user29079 Dec 13 '12 at 16:48

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.

share|improve this answer
    
But my concern is a dirty variable, and guarding against that (such as, like I said, an uninitialized int). It seems to me that it would be possible for the switch statement to reach default in such a situation. –  Swizzlr Dec 13 '12 at 12:45
    
I don't know the definition for Objective-C by heart, but I assumed that a typdef'd enum would be enforced by the compiler. In other words, an "uninitialized" value could only enter the method if your program already exhibits undefined behaviour, and I find it entirely reasonable that a compiler doesn't take that into account. –  Kilian Foth Dec 13 '12 at 12:57
1  
@KilianFoth no, they aren't. Objective-C enums are C enums, not Java enums. Any value from the underlying integral type could be present in the function argument. –  user4051 Dec 13 '12 at 13:02
    
Also, enums could be set in runtime, from integers. So they can contain any value imaginable. –  user29079 Dec 13 '12 at 15:21

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.

share|improve this answer

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.

share|improve this answer
1  
Please not that the average PC programmer typically finds the concept of defensive programming completely alien, since their view of a program is an abstract utopia where nothing that can be wrong in theory will go wrong in practice. Therefore you will get quite diverse answers to your question depending on who you ask. –  user29079 Dec 13 '12 at 15:19
2  
Clang is not confused; it has been explicitly asked to provide all warnings, which includes ones that are not always useful, such as stylistic warnings. (I personally opt into this warning because I don't want defaults in my enum switches; having them means I don't get a warning if I add an enum value and don't handle it. For this reason, I always handle the bad value case outside the enum.) –  Jens Ayton Dec 13 '12 at 18:14
    
@JensAyton It is confused. All professional static analyser tools as well as all MISRA-compliant compilers will give a warning if a switch statement lacks a default statement. Try one yourself. –  user29079 Dec 13 '12 at 19:08
3  
Coding to MISRA-C is not the only valid use for a C compiler, and, again, -Weverything turns on every warning, not a selection appropriate to a particular use case. Not fitting your taste isn’t the same thing as confusion. –  Jens Ayton Dec 13 '12 at 19:46
1  
@Lundin: I'm pretty sure sticking to MISRA-C does not magically make programs safe and bug-free...and i'm equally sure that there is plenty of safe, bug-free C code from people who have never even heard of MISRA. In fact, it's little more than someone's (popular, but not undisputed) opinion, and there are people who find some of its rules arbitrary and sometimes even harmful outside of the context for which they were designed. –  cHao Dec 14 '12 at 14:28

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.

share|improve this answer
    
Firstly, it’s an unspecified value, not undefined behaviour. This means the compiler may assume some other value that’s more convenient, but it may not turn the moon into green cheese, etc. Secondly, as far as I can tell, that only applies to C++. In C, the range of values of an enum type is the same as the range of values of its underlying integer type (which is implementation-defined, hence must be self-consistent). –  Jens Ayton Dec 13 '12 at 18:34
    
However, an instance of an enum variable and an enumeration constant do not necessarily need to have the same width and signedness, they are both impl.defined. But I'm pretty sure that all enums in C are evaluated exactly like their corresponding integer type, so there is no undefined or even unspecified behavior there. –  user29079 Dec 13 '12 at 19:17

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

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

Edit: Let me be more clear. The instant someone passes an uninitialized int into your function, it is UB. 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.

share|improve this answer
2  
How about logging it or throw an exception instead if silently ignore it? –  jgauffin Dec 13 '12 at 13:26
1  
Indeed, there is a lot that can be done, such as... adding a default statement to the switch and gracefully handle the error. This is known as defensive programming. It will not only protect against the dumb programmer handing the function incorrect inputs, but also against various bugs caused by buffer overflow, stack overflow, runaway code etc etc. –  user29079 Dec 13 '12 at 15:07
    
No, it won't handle anything. It is UB. The program has UB the moment the function is entered, and the function body can do nothing about it. –  DeadMG Dec 14 '12 at 13:11
1  
@DeadMG An enum can have any value that its corresponding integer type in the implementation might have. There is nothing undefined about it. Accessing the contents of an uninitialized auto variable is indeed UB, but the "evil hack" (which is more likely some sort of bug) might as well toss a well-defined value to the function, even though it is not one of the values listed in the enum declaration. –  user29079 Dec 14 '12 at 15:47

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!

share|improve this answer

Here's an alternative suggestion: since 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. 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.

share|improve this answer
    
this post is rather hard to read (wall of text). Would you mind editing it into a better shape? –  gnat Jun 3 at 16:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.