[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

xgupta added a comment. Wanted to point out that there is a clang-tidy check in review doing the similar thing - https://reviews.llvm.org/D144429. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142800/new/ https://reviews.llvm.org/D142800

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet updated this revision to Diff 506887. hazohelet added a comment. Herald added a subscriber: jplehr. Rebase and Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142800/new/ https://reviews.llvm.org/D142800 Files: clang/docs/ReleaseNotes.rst

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet added a comment. In D142800#4165090 , @aaron.ballman wrote: > In D142800#4104241 , @hazohelet > wrote: > >>> Also, why are these diagnostics off by default? Do we have some idea as to >>> the false

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet updated this revision to Diff 502166. hazohelet added a comment. Update the differential - Revise warning messages based on ideas from @aaron.ballman. - Introduce a new warning flag `-Wchaining-comparisons` that is enabled by default, to warn about chaining relationals or equal

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

aaron.ballman added a comment. In D142800#4104241 , @hazohelet wrote: >> Also, why are these diagnostics off by default? Do we have some idea as to >> the false positive rate? > > As for the false positive rate, I have checked for instances of this

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet added a comment. > Also, why are these diagnostics off by default? Do we have some idea as to > the false positive rate? As for the false positive rate, I have checked for instances of this warning in the codebases for 'oneapi-src/oneTBB', 'rui314/mold', and 'microsoft/lightgbm',

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6654 +def warn_comp_op_in_comp_op : Warning< + "'%0' within '%1'">, InGroup, DefaultIgnore; +def warn_rel_op_in_rel_op : Warning< Spit-balling wording ideas:

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

erichkeane added a comment. I'm reasonably happy here. I think the chained-comparisons getting the && suggestion is nice. However in each case, the diagnostic is REALLY poor. It isn't really clear to most folks (barely clear to me!) that `'<' within '>'` really means anything. I'd love if

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet updated this revision to Diff 494635. hazohelet added a comment. Address comments from erichkeane: - Fix comment wording - Avoid using macro in test file Implement my proposal for fix-it hint: - In the case of chained relational operators (`<`, `>`, `<=`, `>=`), suggest adding `&&`.

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

dblaikie added a comment. > I'd be interested to see the fixit-hints for the first bit, also to see how > others feel about it here. My 2c is that `-Wparentheses` is already a very stylistic warning (even correct code, without knowing about this esoteric/specific suppression style of adding

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15548 + + SuggestParentheses(Self, Bop->getOperatorLoc(), + Self.PDiag(diag::note_precedence_silence) hazohelet wrote: > erichkeane wrote: > > I find myself wondering

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15548 + + SuggestParentheses(Self, Bop->getOperatorLoc(), + Self.PDiag(diag::note_precedence_silence) erichkeane wrote: > I find myself wondering if we could provide a

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

erichkeane added inline comments. Comment at: clang/test/Sema/comparison-op-parentheses.c:1 +// RUN: %clang_cc1 -fsyntax-only -verify %s -DSILENCE +// RUN: %clang_cc1 -fsyntax-only -verify %s -Wcomparison-op-parentheses don't use macros to control conditional

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

erichkeane added a subscriber: tahonermann. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15539 +/// Emit a diagnostic together with a fixit hint that wraps the inner comparison +/// expression in parentheses. +static void

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet updated this revision to Diff 492986. hazohelet added a comment. Fix the new warning issued against libcxx test code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142800/new/ https://reviews.llvm.org/D142800 Files: clang/docs/ReleaseNotes.rst

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

hazohelet created this revision. hazohelet added reviewers: dblaikie, aaron.ballman, erichkeane. Herald added a project: All. hazohelet requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Herald added a project: clang. This patch introduces