On Wed, Feb 8, 2012 at 3:33 PM, Joerg Sonnenberger <[email protected]> wrote: > On Wed, Feb 08, 2012 at 03:04:44PM -0800, David Blaikie wrote: >> > The critical question is whether (a) a warning finds a useful number of >> > real bugs (b) doesn't have too many false positives and (c) has a >> > reasonable workaround for the cases of (b). >> >> (a) is hard to say because this warning itself wasn't really intended >> to find current bugs - it was intended to ensure you find future bugs >> (when you add an enum value & then fail to update a switch) > > Experience with large projects say that all likely bugs have been made > in the past already :)
Right - well, for whatever that's worth there were lots of real violations of this rule in LLVM - obviously because we actually tried to maintain this style/approach there weren't (m)any deliberate uses of defaults on covered switches. But yeah - more experience with other code bases is important, such as yours. >> (b) hard to say - like -Wparentheses and the original -Wswitch >> warning, this is more a convention designed to communicate extra >> information to the compiler to check semantics that aren't explicit in >> the code > > I agree that this is an edge case. > >> (c) The most immediate is to simply cast the switch condition >> expression to an integer type. This seems like it communicates clearly >> to the compiler "this isn't actually bounded by the enum type - I >> expect it to be any possible integer value". Alternatively, to do the >> error checking before the switch (assert(x >= first_enum && x <= >> last_enum) - and whenever the -Wswitch enum triggers on the following >> switch, it'd be quite close/easy to update the assert too). > > So what is the correct integer type to use for the cast? Keep in mind > it depends on the values of the enum members. std::underlying_type<e>::type > As such, casting doesn't > sound like the correct answer. Sort of seems like it is - if you actually want to tolerate the value being outside the actual enum values that would communicate it clearly to the compiler. > Checking the range also doesn't work in > most interesting cases, since enums aren't always consecutive either. That's fair. > More importantly, both cases just destroys the analysis potential again. > Just because I want to program defensively, doesn't mean the check isn't > desirable. Using __builtin_unreachable() as marker works if there is no > sane error recovery. Still makes it hard to tell whether it's deliberate - when you do add a new enum value what does it mean? Should -Wswitch flag this, or is it just assumed that the user intended that the new enum value would never occur in this switch? > That is good enough for checks deep in the gusts of > code. It doesn't help for code on module borders. If you don't have any control over the bounds/you expect it to have other values I'm still sort of seeing the cast to integral type to describe the situation you're in. I suppose you lose things like the ability to be warned about testing values that aren't any of the enums, or -Wswitch (though these scenarios don't get this functionality at the moment anyway), etc. > This seems > to call for a __builtin_invalid_case() :) Kind of like a semantically > stronger version of the branch prediction statements. I'm not sure it's worth coming to that... ;) - David _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
