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

Wed Feb 12 07:55:44 PST 2020

llunak marked an inline comment as done.
llunak added a comment.

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

> This patch also omitted cfe-commits lists.

That is a Phabricator problem. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface says to select 'Clang' as the repository, but after the monorepo switch that repository no longer exists and Phabricator says 'Inactive' for it. So I (presumably, I don't remember) selected the monorepo and tagged the issue with 'Clang'. If it's still required to use 'Clang' as the repository, then it shouldn't be marked as inactive, or alternatively cfe-commits should be added on the 'Clang' tag.

================
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);
----------------
aaron.ballman wrote:
> 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
As said above, it's on-demand, and in code without unannotated fallthough it'll be triggered exactly zero times.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73852