Boost is not compatible with Clang's -Wimplicit-fallthrough diagnostic

[Alexander Kornienko <alexfh@…>]

Some of Boost source/header files trigger Clang's -Wimplicit-fallthrough warning, which finds unannotated fall-throughs between case labels in switch statements (more details on the diagnostic here: ​http://clang.llvm.org/docs/LanguageExtensions.html#the-clang-fallthrough-attribute).

I propose introducing BOOST_FALLTHROUGH macro to be used to annotate non-trivial fall-through between case labels.

A patch with the macro and fixes to the code is attached.

[Alexander Kornienko <alexfh@…>]

Patch with BOOST_FALLTHROUGH macro and fixes to Boost sources/headers

[viboes]

Adding something to Boost.Config needs tests and documentation. You can find on the documentation how this must be done. If you can provide a complete patch there are more chanes this can be included. Once Boost.Config is updated, you can create a ticket for each one of the concerned libraries.

[anonymous]

For the second part, are you saying this will require a separate ticket and patch for each library that's updated to use the annotation?

[John Maddock]

For the second part, are you saying this will require a separate ticket and patch for each library that's updated to use the annotation?

That would certainly increase the chance of acceptance.

One thing about your patches though: some compilers (GCC?) will warn if there are empty statements - which is exactly what your warning patch will generate on non-clang compilers.

So BOOST_FALLTHROUGH needs to be defined to be used without a following ";".

[Alexander Kornienko <alexfh@…>]

Replying to johnmaddock:

For the second part, are you saying this will require a separate ticket and patch for each library that's updated to use the annotation?

That would certainly increase the chance of acceptance.

Thanks for clarification.

One thing about your patches though: some compilers (GCC?) will warn if there are empty statements - which is exactly what your warning patch will generate on non-clang compilers.

So BOOST_FALLTHROUGH needs to be defined to be used without a following ";".

On unsupported compilers (or in non-C++11 mode in Clang) it expands to "do {} while(0)", which requires a semicolon as well. This construct was meant to look and behave consistently with 'break;' statement, which it can be an alternative to in many cases. So requiring a semicolon after it seems to be reasonable.

[John Maddock]

On unsupported compilers (or in non-C++11 mode in Clang) it expands to "do {} while(0)", which requires a semicolon as well. This construct was meant to look and behave consistently with 'break;' statement, which it can be an alternative to in many cases. So requiring a semicolon after it seems to be reasonable.

"do{}while(0)" generates a "Conditional expression is constant" warning on MSVC.

Defining to "(void)" might do it, but it wouldn't surprise me if some compiler warned about that too in future :-(

[Alexander Kornienko <alexfh@…>]

Replying to johnmaddock:

On unsupported compilers (or in non-C++11 mode in Clang) it expands to "do {} while(0)", which requires a semicolon as well. This construct was meant to look and behave consistently with 'break;' statement, which it can be an alternative to in many cases. So requiring a semicolon after it seems to be reasonable.

"do{}while(0)" generates a "Conditional expression is constant" warning on MSVC.

Defining to "(void)" might do it, but it wouldn't surprise me if some compiler warned about that too in future :-(

How about a separate define for Clang with C++11, and a separate one for MSVC? All others can use "do{}while(0)".

As an idea for compilers that warn on "do{}while(0)", we could use a function call without trailing semicolon:

void boost_fallthrough() {}
#define BOOST_FALLTHROUGH boost_fallthrough()

Then all implementations will require a trailing semicolon, which will help to avoid at least one type of errors in usages of this macro.

[Alexander Kornienko <alexfh@…>]

Patch for Boost.Config: adds BOOST_FALLTHROUGH macro + tests + documentation.

[Alexander Kornienko <alexfh@…>]

I've added another patch (boost-config-fallthrough.patch) with changes only to Boost.Config. It contains BOOST_FALLTHROUGH macro definition for Clang and all other compilers, documentation in .qbk format, compiled .html documentation, tests and introduces one usage of BOOST_FALLTHROUGH macro in libs/config/test/boost_no_std_wstreambuf.ipp. I'm not sure if the last one should go to this patch or not.

I've ran tests for this patch with a recent development version of Clang. I've also verified that ((void)0); construct doesn't warn on MSVC 2012 (v17.00.51025 for x86).

Please review the patch. Thank you!

[viboes]

I think John is right. It would be better if the macro contains the ';' so that there is no risk in introducing a new warning.

#if __cplusplus >= 201103L && defined(__has_warning) 
#  if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough") 
#    define BOOST_FALLTHROUGH [[clang::fallthrough]]; 
#  endif 
#endif

and

#ifndef BOOST_FALLTHROUGH 
#  define BOOST_FALLTHROUGH
#endif 

[Alexander Kornienko <alexfh@…>]

Replying to viboes:

I think John is right. It would be better if the macro contains the ';' so that there is no risk in introducing a new warning.

I'm happy to change the macro to not require a semicolon, but first I'd like to share my arguments for the opposite solution:

1. Consistency with break/return/continue/throw/goto:

   break;
   return xxx;
   BOOST_FALLTHROUGH;
   // but without semicolon it looks alien:
   BOOST_FALLTHROUGH
   

2. More generally, C++ statements usually end with a semicolon or closing brace, and a standalone identifier doesn't look like a separate valid C++ statement; this makes code less readable.

3. For the same reason as 2. this can confuse various tools, such as code editors, code formatters, linters etc. E.g. vim treats the next line after a standalone identifier as a continuation, and adds extra indent to it. clang-format - the new C++ formatter - is not going to format this correctly too.

4. If the macro doesn't require a semicolon, but a developer inserts it by mistake, presence of this extra null-statement will lead to two warnings on Clang with -Wimplicit-fallthrough: fallthrough annotation is not immediately before a fallthrough and fallthrough is not annotated. On other compilers this will only warn if a compiler complains about null-statements. I consider this a worse situation than when a macro requires a semicolon, and all compilers will warn if a developer forgets it.

That's it. Now it's up to you to decide what is better for Boost.

[John Maddock]

I have to be honest: I'm not particularly taken by any of the solutions so far :-(

By way of keeping it simple, what's wrong with just disabling (with pushes and pops) this warning in the few effected source files - rather like we do with MSVC warnings? IMO it looks simpler and clearer to do it this way rather than introducing new macros etc...

[Steven Watanabe]

Replying to johnmaddock:

By way of keeping it simple, what's wrong with just disabling (with pushes and pops) this warning in the few effected source files - rather like we do with MSVC warnings? IMO it looks simpler and clearer to do it this way rather than introducing new macros etc...

+1

[gromer@…]

FWIW I agree with alexfh that the macro should require a semicolon at the point of use. I'm kind of flabbergasted that "while(0)" warns in MSVC; "do{...}while(0)" is an idiomatic way of providing a macro that can be used wherever a statement is expected, and "while(true)" or the equivalent is an even more idiomatic way of writing a loop with no condition. This Stack Overflow answer seems to indicate a consensus that MSVC's warning is broken: ​http://stackoverflow.com/a/1946485

As for the idea of disabling the warning with pragmas, I think that would be a missed opportunity: unintended fall-through is a common source of real bugs, and in places where fall-through is intended, I think explicitly annotating that fact makes the code more readable. If we're going to mark up the source files anyway, why not mark them up in a way that enhances readability and catches bugs?

[Alexander Kornienko <alexfh@…>]

Replying to johnmaddock:

I have to be honest: I'm not particularly taken by any of the solutions so far :-(

By way of keeping it simple, what's wrong with just disabling (with pushes and pops) this warning in the few effected source files - rather like we do with MSVC warnings? IMO it looks simpler and clearer to do it this way rather than introducing new macros etc...

This macro could be a better alternative to fall-through/fall through/FALLTHROUGH/fallthrough/drop through/the lack of break is intentional/... and dozens of other variations of comments annotating this for humans. Frequently, there are no comments at all, so it's sometimes difficult to understand whether a fall-through is intentional or not. Having compiler-verifiable annotations would improve code readability and prevent a common kind of bug when one just forgets to put a 'break;' before next case label.

[John Maddock]

I still worry that this will simply generate more compiler warnings down the road... However, I do like the syntax. So I'll commit shortly.

[John Maddock]

(In [83958]) Apply patch from #8408. Fixes #8408.

[Alexander Kornienko <alexfh@…>]

Thank you! Nit: there are trailing spaces added almost in each changed line, I guess, this is not intentional.

[John Maddock]

Nit: there are trailing spaces added almost in each changed line, I guess, this is not intentional.

My bad. Does it matter?

[Alexander Kornienko <alexfh@…>]

Replying to johnmaddock:

Nit: there are trailing spaces added almost in each changed line, I guess, this is not intentional.

My bad. Does it matter?

As a minor style issue + a bit of extra work for us when merging.

[gromer@…]

FWIW, speaking as the one who'll probably be responsible for the merging, I have no problem with the trailing whitespace, and the style issue seems like a moot point, since trailing whitespace is pretty endemic to the Boost codebase.

[Alexander Kornienko <alexfh@…>]

Then it doesn't matter.