[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-08-02 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. I've posted https://reviews.llvm.org/D85097 to replace this review. https://reviews.llvm.org/D85097 implements this check in the CFE instead of as a tidy check per recommendation from @lebedev.ri . If acceptable, I'll abandon this review. Repository: rG LLVM Gith

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-08-01 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. Looks like this can be implemented as a warning in the cfe (as @lebedev.ri suggested). I'll probably abandon this review, but will keep it active until I have the alternative cfe warning patch prepared. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-08-01 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. @njames93, I'll take a crack at implementing a cfe diagnostic for this, see how far I get. Cheers :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-08-01 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D84898#2184931 , @vabridgers wrote: > I believe all review comments have been address, except for the discussion on > implementing this in the CFE or as a tidy check. Could try firing off an RFC to cfe-dev see if other people

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers marked 2 inline comments as done. vabridgers added a comment. Thanks Eugene, I think the backtick issue has been addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 ___

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers updated this revision to Diff 281943. vabridgers added a comment. Address backticks in rst files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 Files: clang-tools-extra/clang-tidy/bugprone/Bug

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Eugene Zelenko via Phabricator via cfe-commits
Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:75 + + Finds complex conditions using `<`, `>`, `<=`, and `>=` that could be + interpreted ambiguously. Double back-ticks, not single ones. Comment at:

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. I believe all review comments have been address, except for the discussion on implementing this in the CFE or as a tidy check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 __

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers updated this revision to Diff 281937. vabridgers marked 6 inline comments as done. vabridgers added a comment. Address most review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 Files:

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. Thanks all for the prompt and actionable comments. I will address all comments directly and 1-1, but would like to bottom out on one point before driving this forward. @lebedev.ri commented this should be in the Clang front end rather than a tidy check. Can we reach

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. For the record `X < Y < Z ` does have a mathematical meaning, Y is constrained between X and Z. However in the context of `C` the expression isnt parsed like that. If someone writes this they likely wanted `(X < Y) && (Y < Z)` For this specific check as you pointed out w

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-30 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. I really think this should be in clang proper. I have seen such patterns written in the wild, they should be caught by just the compiler. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/

[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

2020-07-29 Thread Eugene Zelenko via Phabricator via cfe-commits
Eugene.Zelenko added a comment. May be this check belongs to `bugprone` module? Comment at: clang-tools-extra/docs/ReleaseNotes.rst:75 + + Finds complex conditions using <, >, <=, and >= that have no mathematical + meaning. Please enclose comparison operator

[PATCH] D84898: clang-tidy] Add new checker for complex conditions with no meaning

2020-07-29 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers added a comment. I believe this is ready for review. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D84898: clang-tidy] Add new checker for complex conditions with no meaning

2020-07-29 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers updated this revision to Diff 281770. vabridgers added a comment. Another update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt clang-

[PATCH] D84898: clang-tidy] Add new checker for complex conditions with no meaning

2020-07-29 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers updated this revision to Diff 281769. vabridgers added a comment. Correct some simple mistakes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84898/new/ https://reviews.llvm.org/D84898 Files: clang-tools-extra/clang-tidy/misc/CMakeList

[PATCH] D84898: clang-tidy] Add new checker for complex conditions with no meaning

2020-07-29 Thread Vince Bridgers via Phabricator via cfe-commits
vabridgers created this revision. Herald added subscribers: cfe-commits, phosek, Charusso, mgorny. Herald added a project: clang. vabridgers requested review of this revision. This checker finds cases where relational expressions have no meaning. For example, (x <= y <= z) has no meaning, but just