[llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC (original) (raw)

Matthias Braun via llvm-dev llvm-dev at lists.llvm.org
Wed Oct 31 16:16:21 PDT 2018

+1! Accidental fallthrough is a common bug class in my experience and this a great tool to avoid them.

In reality you already get mail from some buildbots anyway when you forget LLVM_FALLTHROUGH AFAIK so this just codifies existing practice.

On Oct 31, 2018, at 3:25 PM, Justin Bogner via llvm-dev <llvm-dev at lists.llvm.org> wrote:

+1. I tried to do this a couple of years ago (not knowing about the proposal in 2012) but there was too much to annotate at the time. It seems like this is easy to do now. Reid Kleckner via llvm-dev <llvm-dev at lists.llvm.org> writes: Alex Kornienko proposed enabling this warning back in 2012 here: http://lists.llvm.org/pipermail/llvm-dev/2012-July/051386.html

At the time, Chris Lattner said he didn't feel it was worth annotating all of LLVM and Clang with a new macro to enable this warning. However, GCC 7 added this warning as part of -Wextra, and we've slowly annotated most of LLVM and Clang with LLVMFALLTHROUGH. At this point, I think we should re-evaluate this decision and enable this warning for both clang and GCC. Since our codebase is already annotated, it will help developers (like me), who use Clang locally, to find unintended fallthrough bugs in their code. For example, I committed r345676, which had to be reverted because of an unintended fallthrough. This warning would've helped. There is also marginal benefit to aligning warnings between GCC and Clang. While there will always be divergence in warnings between GCC and Clang, when possible, it saves time when clang can diagnose things that would later become a -Werror warning on some GCC 7 buildbot. This is a summary of differences in the behavior of -Wimplicit-fallthrough between clang and GCC: 1. GCC recognizes comments that say "fall through" as annotations, clang doesn't 2. GCC doesn't warn on "case N: foo(); default: break;", clang does 3. GCC doesn't warn when the case contains a switch, but falls through the outer case. See the AArch64ISelLowering.cpp change for an instance where this almost caused a bug, but a redundant check saved us. I've removed the redundant check. 4. Clang warns on LLVMFALLTHROUGH after llvmunreachable. GCC doesn't, so I removed the one instance of this that I found. Changing Clang's behavior in light of these differences is out of scope for me. I want developers who compile with any of the last 4 years of clang releases to be able to use this warning, and those releases have the behavior described above. If you want to discuss changing Clang to be more like GCC here, please file a bug or start a thread on cfe-dev. I posted a patch with the this RFC as the commit message here so you can see what this looks like now: https://reviews.llvm.org/D53950 To summarize, this warning is already enabled for GCC and we've already annotated most of our codebase for it, so let's enable the warning for clang.

LLVM Developers mailing list llvm-dev at lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

More information about the llvm-dev mailing list