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

tbaeder added a comment. I know this has been reverted, but for the record, the implementation has a few shortcomings that makes it hard to use it for projects that rely on fallthrough comments: - The comment is not recognized if it is on the same line as the last statement, e.g. case 2:

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

llunak added a comment. In D73852#1901895 , @rsmith wrote: > Thank you, Luboš, and sorry for the process problems here. FWIW, the Clang repository here in Phabricator is still 'Inactive', so even though

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

rsmith added a comment. In D73852#1901836 , @aaron.ballman wrote: > FYI: it was reverted by Luboš in c61401b89742f230b7e6a2cc0548fbf7442e906d > Thank you, Luboš, and sorry for the

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

aaron.ballman added a comment. In D73852#1901793 , @rsmith wrote: > In any case, none of this has any bearing on whether this patch has had > sufficient review. It hasn't, so it needs to be reverted for now. FYI: it was reverted by Luboš in

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

rsmith added a comment. In D73852#1901699 , @llunak wrote: > In D73852#1901186 , @rsmith wrote: > > > We shouldn't enable the warning under -Wextra in language modes where > > there's no standard way to suppress

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

rsmith added a comment. Per http://llvm.org/PR43465#c37, this patch has not had proper review and we have not established consensus that we want this. Please revert for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73852/new/

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

llunak added a comment. In D73852#1901186 , @rsmith wrote: > We shouldn't enable the warning under -Wextra in language modes where there's > no standard way to suppress it. That may be true, but that is not what the bugreport is about, it explicitly

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

On Mon, Mar 2, 2020 at 11:42 AM Richard Smith - zygoloid via Phabricator wrote: > > rsmith added a comment. > > I'm also pretty concerned about using comments to drive warning behavior. We > discussed this when first adding our gallery fallthrough warning and its > suppression mechanism and

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

rsmith added a comment. I'm also pretty concerned about using comments to drive warning behavior. We discussed this when first adding our gallery fallthrough warning and its suppression mechanism and made an explicit decision that we did not want comments to affect our behaviour. I don't think

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

nickdesaulniers added a comment. Doesn't GCC also support multiple different levels of this warning for all kinds of different spellings? https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ I have concerns about the performance of this warning. We're going to parse

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

llunak marked an inline comment as done. llunak added a comment. In D73852#1872013 , @lebedev.ri wrote: > This patch also omitted cfe-commits lists. That is a Phabricator problem.

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

xbolva00 added a comment. I think that this patch is needed since we need to deal with this situation somehow because fallthru comments are used in many real world code. But I am personally against extending it more and use comments to disable warnings generally. Repository: rG LLVM Github

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

aaron.ballman added a comment. In D73852#1872104 , @aaron.ballman wrote: > All this said, I am comfortable reverting this back out of the 10.0 branch > while we consider it harder. It does not seem so critical that we can't take > time to discuss it

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

aaron.ballman added a comment. In D73852#1872064 , @thakis wrote: > In D73852#1872044 , @aaron.ballman > wrote: > > > In D73852#1872000 , @thakis wrote: > > > > > I think

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

thakis added a comment. In D73852#1872044 , @aaron.ballman wrote: > In D73852#1872000 , @thakis wrote: > > > I think giving comments a semantic meaning is a bad idea. Do we really have > > to do this? > > > I

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

aaron.ballman added a comment. In D73852#1872019 , @hans wrote: > I also jumped when I saw that this now makes certain comments "load bearing". > That doesn't seem like a great idea to me. It's an idea we already have in the project though with NOLINT

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

aaron.ballman added a comment. In 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

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

lebedev.ri added a comment. Also, rG398b wasn't sent to cfe-dev lists. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73852/new/ https://reviews.llvm.org/D73852 ___ cfe-commits mailing list

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

hans added a comment. I also jumped when I saw that this now makes certain comments "load bearing". That doesn't seem like a great idea to me. The warning may be all right for C++ code, which has an attribute to suppress it, but C code does not normally use such attributes, and has no standard

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

lebedev.ri added a comment. Now this patch should be reverted. This patch also omitted cfe-commits lists. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73852/new/ https://reviews.llvm.org/D73852 ___

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

thakis added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1239 + llvm::Regex("(/\\*[ \\t]*fall(s | |-)?thr(ough|u)\\.?[ \\t]*\\*/)" + "|(//[ \\t]*fall(s | |-)?thr(ough|u)\\.?[ \\t]*)", +

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

thakis added a comment. I think giving comments a semantic meaning is a bad idea. Do we really have to do this? In addition to this making comments have semantic meaning, the meaning is different from gcc. I don't think we should support this. Repository: rG LLVM Github Monorepo CHANGES

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG398b4ed87d48: [clang] detect switch fallthrough marked by a comment (PR43465) (authored by llunak). Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST