[cfe-commits] r150055 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Sema/switch.c

[Matthieu Monrocq]

Le 9 février 2012 01:00, David Blaikie <dblaikie at gmail.com> a écrit :

> On Wed, Feb 8, 2012 at 3:33 PM, Joerg Sonnenberger
> <joerg at britannica.bec.de> 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
>
>
Ah true, I had forgotten about the issue with defensive programming.

I had admit that my personal style is *not* to fall out from a switch over
an enum (but return instead) so that a fallout is necessarily "invalid",
but I can definitely understand that not all switches are so coded, many
people like having big functions ;)

I will let the discussion of policy to people with a much larger experience
on either CLang or very large code bases, and in exchange I'd like to spawn
off the discussion of how to "ressucitate" the original goal of the
-Wswitch warning:

=> detecting switches over an enum where one (a few ?) enumerators have
been forgotten

At the moment -Wswitch is not triggered whenever there is a default clause.
Still as we saw, this default clause can be there purely because the values
of the enum are not necessarily limited to the enumerators (sigh), and may
not indicate that *all other* values have been *consciously* discarded.

There are two things that hints that this is not conscious:
1. A single enumerator is missing
2. And the file containing the switch is (significantly, say a few seconds)
older than the file containing the enum

I think the first point is quite reasonnable. Using a default clause to
discard a *single* enumerator seems dubious, at worst it can simply be
enumerated right before the default clause easily enough.

The second point restricts this first to protect from installed libraries
(where all files are copied in a matter of second) and really center around
human error. A simple *touch* on the file containing the switch solves the
issue. With this protection in place, we might be willing to move from a
*single* missing enumerator toward a *few* (10% ?) to cover the case where
one adds several enumerators at once.

On the other hand, I do not know if we want to introduce a dependency on
file timestamps to produce warnings, I would certainly understand a certain
reluctance here!

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120209/a22bdfb7/attachment.html>