Boost C++ Libraries: Ticket #8408: Boost is not compatible with Clang's -Wimplicit-fallthrough diagnostic

attachment set

Fri, 05 Apr 2013 23:53:03 GMT

attachment → boost-fallthrough.diff

Patch with BOOST_FALLTHROUGH macro and fixes to Boost sources/headers

component changed; owner set

viboes — Sun, 07 Apr 2013 18:35:15 GMT

owner set to John Maddock
component None → config

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.

cc set

gromer@… — Mon, 08 Apr 2013 15:18:08 GMT

cc gromer@… added

anonymous — Mon, 08 Apr 2013 15:20:48 GMT

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 — Mon, 08 Apr 2013 16:59:22 GMT

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 ";".

Mon, 08 Apr 2013 17:38:53 GMT

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 — Mon, 08 Apr 2013 17:56:44 GMT

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 :-(

Mon, 08 Apr 2013 18:31:38 GMT

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.

attachment set

Tue, 09 Apr 2013 18:39:39 GMT

attachment → boost-config-fallthrough.patch

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

Tue, 09 Apr 2013 18:47:53 GMT

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 — Tue, 09 Apr 2013 19:02:19 GMT

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

Wed, 10 Apr 2013 00:40:18 GMT

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:

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

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

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.

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.

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 — Wed, 10 Apr 2013 12:24:34 GMT

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 — Wed, 10 Apr 2013 14:22:05 GMT

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...

gromer@… — Wed, 10 Apr 2013 14:45:04 GMT

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?

Wed, 10 Apr 2013 16:41:02 GMT

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 — Thu, 18 Apr 2013 17:48:02 GMT

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.

status changed; resolution set

John Maddock — Thu, 18 Apr 2013 17:50:22 GMT

status new → closed
resolution → fixed

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

Thu, 18 Apr 2013 22:25:38 GMT

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

John Maddock — Mon, 22 Apr 2013 17:52:14 GMT

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

My bad. Does it matter?

Mon, 22 Apr 2013 18:50:03 GMT

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@… — Mon, 22 Apr 2013 19:15:46 GMT

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.

Mon, 22 Apr 2013 20:07:46 GMT

Then it doesn't matter.

version, milestone changed

James E. King, III — Thu, 12 Oct 2017 17:44:03 GMT

version Boost Development Trunk → Boost 1.61.0
milestone To Be Determined → Boost 1.62.0