[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

Wed Feb 12 05:45:56 PST 2020

aaron.ballman added a comment.

In D73852#1872000 <https://reviews.llvm.org/D73852#1872000>, @thakis wrote:

> I think giving comments a semantic meaning is a bad idea. Do we really have to do this?

I think that ship has sailed, for instance, see // NOLINT comments that are also supported to silence diagnostics. Also, the original bug report mentions that flex generates code using these constructs, so there are real world libraries using this construct.

> In addition to this making comments have semantic meaning, the meaning is different from gcc.

We're covering the cases GCC does that make sense within Clang -- or have we deviated from GCC in a way that is significant to our users?

> I don't think we should support this.

I think we should -- C did not have any other way to resolve this issue without C2x support or compiler extensions, and fallthrough is definitely an issue in C. While it's not my favorite solution to silencing the diagnostics, it solves a real problem that C programmers are hitting.

In D73852#1872013 <https://reviews.llvm.org/D73852#1872013>, @lebedev.ri wrote:

> Now this patch should be reverted.
>  This patch also omitted cfe-commits lists.

That is not an automatic reason to revert a patch, especially one that has been accepted by a code owner.

================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1239
+              llvm::Regex("(/\\*[ \\t]*fall(s | |-)?thr(ough|u)\\.?[ \\t]*\\*/)"
+                          "|(//[ \\t]*fall(s | |-)?thr(ough|u)\\.?[ \\t]*)",
+                          llvm::Regex::IgnoreCase);
----------------
thakis wrote:
> Also, this adds a regex match for every comment line, yes? Isn't this terrible for build performance? Did you do any benchmarking of this?
https://reviews.llvm.org/D73852#inline-671309

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73852/new/

https://reviews.llvm.org/D73852