should -Wimplicit-fallthrough require C++11?

Hi all,

In response to numerous internal complaints at Apple from C/Objective-C users, I've gone and disabled -Wimplicit-fallthrough when using straight C (not C++):

  http://llvm.org/viewvc/llvm-project?view=rev&revision=167655

The motivation is that the attribute used to decorate a fall-through as explicit is only available when using C++11. This was particularly frustrating for C users.

I understand that change alone might be contentious, and obviously if there is serious concern that should be discussed. I'd like to one step further and ask whether or not this warning should even be on when using C++98? The same argument applies there as well; the attribute to decorate a fall-through as explicit is not available.

Indeed, I would have gone and restricted this warning to C++11, but apparently there is a test case specifically for the C++98 scenario:

  test/SemaCXX/switch-implicit-fallthrough-cxx98.cpp

There was a lot of discussion about this warning when it first went in Now that the warning is getting real living-on, I was wondering if we can discuss whether this is desired behavior? It feels somewhat dubious to me to have a warning like this that suggests adding an annotation when no such annotation is available.

Thoughts? Comments?

Cheers,
Ted

I feel like what we really want is "on by default in C++11, off by default in C and C++03", but our warning options don't support that right now. I could see this being a useful warning for codebases that want to have NO fallthroughs.

Jordan

Hi Ted,

Hi all,

In response to numerous internal complaints at Apple from C/Objective-C users, I've gone and disabled -Wimplicit-fallthrough when using straight C (not C++):

  http://llvm.org/viewvc/llvm-project?view=rev&revision=167655

The motivation is that the attribute used to decorate a fall-through as explicit is only available when using C++11. This was particularly frustrating for C users.

I'm not in favor of this change. If people are frustrated that they
can't annotate fallthrough outside C++11, how about we add a
__builtin_fallthrough() which generates no code, and has the same
effect as the [[clang::fallthrough]] attribute? If warning is
considered valuable enough for these users to enable it in C++11 code,
presumably it's valuable enough to be worth making usable outside
C++11 mode.

I understand that change alone might be contentious, and obviously if there is serious concern that should be discussed. I'd like to one step further and ask whether or not this warning should even be on when using C++98? The same argument applies there as well; the attribute to decorate a fall-through as explicit is not available.

Indeed, I would have gone and restricted this warning to C++11, but apparently there is a test case specifically for the C++98 scenario:

  test/SemaCXX/switch-implicit-fallthrough-cxx98.cpp

There was a lot of discussion about this warning when it first went in Now that the warning is getting real living-on, I was wondering if we can discuss whether this is desired behavior? It feels somewhat dubious to me to have a warning like this that suggests adding an annotation when no such annotation is available.

[The warning shouldn't suggest adding an annotation outside C++11. If
it does, it's a bug.]

The original intention was that the warning could be used in any
language mode, if you wanted a warning on *all* switch fallthrough,
and that people who didn't want that could just not turn it on. I
guess the complaints you're receiving are for situations where the
warning can't simply be disabled (or not enabled in the first place)?
If we really can't find a better solution for these people than
disabling the warning in languages where we can't annotate
fallthrough, then we should do so consistently and disable it in C++98
too.

The motivation is that the attribute used to decorate a fall-through as explicit is only available when using C++11. This was particularly frustrating for C users.

I’m not in favor of this change. If people are frustrated that they
can’t annotate fallthrough outside C++11, how about we add a
__builtin_fallthrough() which generates no code, and has the same
effect as the [[clang::fallthrough]] attribute?

I like this idea, but that raw syntax to propose to users is hideous. If we could wrap it in something consistent for both C and C++98/C++11 users, e.g. a macro that expands to the “right thing”, that would be more appealing.

If warning is
considered valuable enough for these users to enable it in C++11 code,
presumably it’s valuable enough to be worth making usable outside
C++11 mode.

I agree.

The original intention was that the warning could be used in any
language mode, if you wanted a warning on all switch fallthrough,
and that people who didn’t want that could just not turn it on.

Right.

I
guess the complaints you’re receiving are for situations where the
warning can’t simply be disabled (or not enabled in the first place)?

What has happened is that a fair number of users have discovered this warning using -Weverything. When they first discover it they find it interesting. After they discover that they cannot add an annotation (as the warning suggests) they get very frustrated. They are then left with the choice of just turning the warning off, or using pragmas. For those users who like the warning in principle, but find that they cannot use it in practice because of these limitations (because they are not using C++11) they view it is an incomplete feature. Several people have thus requested to not see the warning at all.

If we really can’t find a better solution for these people than
disabling the warning in languages where we can’t annotate
fallthrough, then we should do so consistently and disable it in C++98
too.

Agreed, but I like your suggestion of using something like a builtin. Right now this warnings feels like an incomplete feature. It has a really great workflow in C++11 with the use of the attribute. Otherwise, unless the user wants a zero-tolerance policy with this warning, the workflow is not great. What I am afraid of is people turning this warning off (either directly or by never turning it on) when they may have found it useful. Once they make that decision, it is unlikely they will give the warning a second try.

You may have noticed that I reverted my change, pending further discussion here. I still think we should consider what to do for the LLVM 3.2 branch, in case this doesn’t get enough attention before then. I myself won’t have any cycles over the weekend to look at this. A builtin, however, doesn’t sound so hard to implement, but I’d prefer some way to sugar coat this to avoid the ugly syntax.

Yes, this is where it gets tricky. In C++11 we had a nice syntactic
slot to put something non-ugly. Nonetheless, we expected people to
wrap it in some kind of compatibility macro, and I think we would have
the same expectation of whatever solution we pick for the
outside-C++11 case. Given that, perhaps it's not terrible to pick
something ugly?

If you think the aesthetics of the raw annotation really matters, I
think the least ugly option open to us would be to predefine a
"__fallthrough" macro, which could expand to __builtin_fallthrough(),
or to some keyword (perhaps even a __fallthrough keyword). Having this
be detectable by the preprocessor seems useful, so that people can
easily write portable code:

#ifdef __fallthrough
#define fallthrough __fallthrough
#else
#define fallthrough do {} while (0)
#endif

(With the C++11 attributes approach, we were using
__has_warning("-Wimplicit-fallthrough") for this.) That said, I view
the bar for a new keyword or predefined macro as being higher than
that for a new builtin, in part due to the increased risk of breaking
existing code.

OK, I'm no longer quite so worried about breaking code with the
predefined macro... I've done some searching, and the *only* uses I've
found of __fallthrough as an identifier were cases where it was being
used for exactly this purpose already. It seems that Microsoft's SAL
defines it as a macro for exactly this purpose.

The original intention was that the warning could be used in any
language mode, if you wanted a warning on *all* switch fallthrough,
and that people who didn't want that could just not turn it on.

Right.

I
guess the complaints you're receiving are for situations where the
warning can't simply be disabled (or not enabled in the first place)?

What has happened is that a fair number of users have discovered this
warning using -Weverything. When they first discover it they find it
interesting. After they discover that they cannot add an annotation (as the
warning suggests) they get very frustrated. They are then left with the
choice of just turning the warning off, or using pragmas. For those users
who like the warning in principle, but find that they cannot use it in
practice because of these limitations (because they are not using C++11)
they view it is an incomplete feature. Several people have thus requested
to not see the warning at all.

If you're using -Weverything, shouldn't the expectation be that you'll
end up turning off several warnings?

The original intention was that the warning could be used in any
language mode, if you wanted a warning on all switch fallthrough,
and that people who didn’t want that could just not turn it on.

Right.

I
guess the complaints you’re receiving are for situations where the
warning can’t simply be disabled (or not enabled in the first place)?

What has happened is that a fair number of users have discovered this
warning using -Weverything. When they first discover it they find it
interesting. After they discover that they cannot add an annotation (as the
warning suggests) they get very frustrated. They are then left with the
choice of just turning the warning off, or using pragmas. For those users
who like the warning in principle, but find that they cannot use it in
practice because of these limitations (because they are not using C++11)
they view it is an incomplete feature. Several people have thus requested
to not see the warning at all.

If you’re using -Weverything, shouldn’t the expectation be that you’ll
end up turning off several warnings?

Yes, it is. Whilst turning warnings has usually been a “white-listing” operation, -Weverything turns it into a “black-listing” operation. However the concern here is that people rarely, if ever, reevaluate the warnings they turn on/off (at least, not until they hit a bug and wonder if it could have gotten caught). Therefore, by having everyone use -Wno-implicit-fallthrough there is a risk that the warning will become useless… even after people switch from C++03 to C++11.

– Matthieu

Yes, but only if those warnings aren't useful to your situation. This warning should have widespread applicability, but it's actually useless outside of C++11 because there's no clean workaround.

  - Doug

I disagree; as a stylistic warning -Wimplicit-fallthrough is still useful. I can definitely see a project where you'd prefer to disallow all fallthroughs and require copied code rather than annotating with a comment.

But we don't really have a good policy for purely-style warnings (besides "they could go in the analyzer instead").

Jordan

I can certainly imagine someone coming up with this rule, but I can't imagine myself ever agreeing that it's the right thing to do :slight_smile:

Copy-and-pasting code is not, in my opinion, a reasonable way to silence a warning. We need something better; __fallthrough would be fine by me.

  - Doug

For my 2c: This warning was only added because we had a way to
annotate. In contexts where there is no such annotation the warning
doesn't really meet Clang's bar. Rather than forcing people to turn
this warning off depending on what they are compiling I think it makes
sense for it to be a no-op in any language variant where we don't
currently have an annotation mechanism.

(this means, imho, for the 3.2 release, we should do as Ted started &
have this disabled in both C and C++98 - unless someone's going to
whip up the annotation device necessary for those languages in very
short order (yes, I realize we've still got weeks until 3.2 actually
releases, but I don't see sufficient benefit to rushing through such a
change))

- David

I completely agree - that's the right thing for the 3.2 release.

-Chris

I agree as well. I have re-implemented this restriction in r167749, but further restricted the warning only to C++11.