Make fallthroughs explicit in lsqpack.c

[r-barnes]

Using __attribute__((fallthrough)); informs the compiler, in a way it understands, that the fallthrough is explicit and wanted.

This allows for the enablement of -Wimplicit-fallthrough, which throws warnings/errors for implicit fallthroughs, of which several exist in this file. Using -Wimplicit-fallthrough prevents unwanted fallthroughs.

[litespeedtech]

Still prefer /* fall through */ over __attribute__((fallthrough)); as it is more portable for different compilers/platforms.

[r-barnes]

@litespeedtech - We could use a macro instead, if that seems better to you, but I do think the fallthrough should be marked using something standards compliant so that compilers know how to interpret it.

[r-barnes]

This macro, for instance, could work: https://github.com/hercules-team/augeas/pull/836/files

#ifndef ATTRIBUTE_FALLTHROUGH
#  if defined __has_attribute
#    if __has_attribute (fallthrough)
#      define ATTRIBUTE_FALLTHROUGH __attribute__ ((fallthrough))
#    else
#      define ATTRIBUTE_FALLTHROUGH
#    endif
#  endif
#endif

[litespeedtech]

but I do think the fallthrough should be marked using something standards compliant so that compilers know how to interpret it. point taken, on the other hand, we want to avoid the standard that is too new, or not widely supported. /* fall through */ are added to silent those compiler warnings, and it works as well as __attribute__ ((fallthrough)).

[r-barnes]

@litespeedtech - I hear you wanting to maintain readability and compatibility with older compilers.

The macro I included above maintains readability while providing a mechanism that newer compilers can use to insert a standards-compliant explicit fallthrough marking; older compilers will be unaffected. This can be used to make the code interoperable with codebases using -Wimplicit-fallthrough.

I'm afraid /* fall through */ does not silence all compiler warnings. I see that it does so for GCC, but that isn't the case for LLVM, per this godbolt. Using such a macro means that the code can be compiled safely with both GCC and LLVM.

[r-barnes]

I've updated the PR to use deliberate_fallthrough as the macro.

[r-barnes]

As an update, there are implicit fallthroughs without comments before them here:

1. link
2. link
3. link

Are these meant to fallthrough? Even if you don't think supporting LLVM's -Wimplicit-fallthrough mechanism is something you'd like to do, adding fall through comments here (or fixing the uncovered bug) would be useful.

[dtikhonov]

What you are proposing, @r-barnes, seems like busywork. The compiler already does the right thing regarding fall-throughs. This code works fine without -Wimplicit-fallthrough: so don't turn this warning on. Problem solved.

Also, be mindful that beauty is in the eye of the beholder!

[r-barnes]

@dtikhonov I'm afraid enabling the -Wimplicit-fallthrough flag is non-optional for the code I'm working with: implicit fallthroughs have a 30-50% bug rate and having the compiler enforce annotations for them is the only scalable way of preventing problems. An upstream fix to your code is valuable because it allows it to integrate with codebases where safety is important.

From a language standards perspective, explicit fallthrough indications are part of the C23 language standard, so I anticipate both that the use of comments as a fallthrough annotation will decrease over time in favour of standardization.

Other large projects have found it valuable to remove implicit fallthroughs and have, repeatedly, found bugs. It's not busy work to ensure one's code is safe and can communicate its safety. Even if you're not on board with the idea of using a more standard way to communicate fallthroughs, your code includes unannotated fallthroughs which might be bugs (see the three links a couple of messages up). I wish you would clarify whether or not these fallthroughs are intentional.

• The Linux kernel eliminated all implicit fallthroughs and no longer allows them (link, link).
• Google's C++ style guide forbids implicit fallthrough (link).
• Searching Github you can find 45.1k merged PRs for "missing break" (link), 4.1k when adding a C++ qualifier (link), and 1.1k with a C qualifier (link).
• Chromium no longer allows implicit fallthroughs (link). This change list shows a number of detected bugs if you search for "missing break" and "unintentional".

[r-barnes]

I'm still hoping for clarification on whether or not these fallthroughs are intentional:

https://github.com/litespeedtech/ls-qpack/blob/8ff2bf67196c8e959dfa8a26e7b6fe1067bef62a/lsqpack.c#L3422

https://github.com/litespeedtech/ls-qpack/blob/8ff2bf67196c8e959dfa8a26e7b6fe1067bef62a/lsqpack.c#L3434

https://github.com/litespeedtech/ls-qpack/blob/8ff2bf67196c8e959dfa8a26e7b6fe1067bef62a/lsqpack.c#L3448

[dtikhonov]

Yes: the code is obvious about it, with names like "state" and "resume."

[r-barnes]

@dtikhonov : I didn't find it to be obvious. I've put up #58 to add fallthrough comments in the style you said you preferred.

[litespeedtech]

Made some updates regarding this, please check the latest code. should make everybody happy. :-)

[dtikhonov]

@r-barnes which compiler are you using? Here is an example where no warning is produced by either gcc or clang: https://godbolt.org/z/MG1q8964f

Was there actually a warning that your change fixed?

[dtikhonov]

Алё гараж? @r-barnes? Show me the warnings. What was this change about?