[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8fbd1157deb: [clang-tidy] Ignore concepts in 
`misc-redundant-expression` (authored by Izaron).

Changed prior to commit:
  https://reviews.llvm.org/D122078?vs=466271=466273#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
@@ -0,0 +1,11 @@
+// RUN: clang-tidy %s -checks=-*,misc-redundant-expression -- -std=c++20 | count 0
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -164,6 +164,12 @@
   :doc:`readability-simplify-boolean-expr `
   when using a C++23 ``if consteval`` statement.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), unless(isInTemplateInstantiation()),
+  

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 466271.
Izaron added a comment.

Removed redundant comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
@@ -0,0 +1,11 @@
+// RUN: clang-tidy %s -checks=-*,misc-redundant-expression -- -std=c++20 | count 0
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), unless(isInTemplateInstantiation()),
+  unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -936,7 +945,8 @@

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.

LGTM, just one small nit.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp:3
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.

This comment isn't strictly necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

Hi! A friendly ping =)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 461003.
Izaron added a comment.

Fix test with `count 0`, thanks to @njames93 !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-tidy %s -checks=-*,misc-redundant-expression -- -std=c++20 | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), unless(isInTemplateInstantiation()),
+  unless(hasAncestor(BannedAncestor)))
   

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D122078#3797519 , @Izaron wrote:

> The new file was failing with message
>
>   CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input
>
> So I had to add an additional urrelevant check.
> I added a check for consteval function (there were no such tests previously).

You don't need the unrelated tests, you can use a special run line that will 
check that no warnings are emitted.
`// RUN: clang-tidy %s --checks=-*,misc-redundant-expression -- | count 0`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 460990.
Izaron added a comment.

The new file was failing with message

  CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input

So I had to add an additional urrelevant check.
I added a check for consteval function (there were no such tests previously).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s misc-redundant-expression -std=c++20 %t -- -- -fno-delayed-template-parsing
+
+consteval int TestOperatorConfusion(int X) {
+  X = (X << 8) & 0xff;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: ineffective bitwise and operation
+  return X;
+}
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 460976.
Izaron added a comment.

Created redundant-expression-cxx20.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s misc-redundant-expression -std=c++20 %t -- -- -fno-delayed-template-parsing
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), unless(isInTemplateInstantiation()),
+  unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -936,7 +945,8 @@

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Nathan James via Phabricator via cfe-commits]

njames93 requested changes to this revision.
njames93 added a comment.
This revision now requires changes to proceed.

Just address that test issue then it'll be good




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp:1
-// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- 
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s misc-redundant-expression -std=c++20 %t -- -- 
-fno-delayed-template-parsing
 

This is removing test coverage for previous language versions unnecessarily. 
Can you create a new test file redundant-expression-cxx20 instead and move this 
new test case into there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 460209.
Izaron added a comment.

Add release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s misc-redundant-expression -std=c++20 %t -- -- -fno-delayed-template-parsing
 
 typedef __INT64_TYPE__ I64;
 
@@ -843,3 +843,13 @@
 
   return 2;
 }
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression `
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -440,6 +441,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -858,6 +861,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -873,7 +877,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -886,7 +891,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -896,7 +902,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -909,7 +916,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -919,7 +927,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), 

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D122078#3407984 , @aaron.ballman 
wrote:

> Okie dokie, thanks for weighing in! This LGTM (feel free to add the release 
> note when landing). If @alexfh has some technical concerns with 
> `hasAncestor()`, we can always revert and address them once we know more.

I feel like this is another issue which could be solved with traversal scopes. 
`isInEvaluationContext` This traversal scope wouldn't traverse things like 
concepts, static_asserts(?), Noexcept & explicit specifiers, sizeof, decltype 
etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D122078#3407981 , @sammccall wrote:

> I don't actually know a great deal about matcher performance :-( I wish I did.
> I think this is pretty much in line with how plenty of check matchers already 
> work though, in particular unless(isInTemplateInstantiation()) is basically 
> the same thing and is used in many places including this check.
> So I wouldn't be particularly worried.
>
> (Obviously from first principles you'd prefer to prune the subtree rather 
> than match within it and then walk up the parent chain, but I don't think 
> matchers supports this pattern well).

Okie dokie, thanks for weighing in! This LGTM (feel free to add the release 
note when landing). If @alexfh has some technical concerns with 
`hasAncestor()`, we can always revert and address them once we know more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I don't actually know a great deal about matcher performance :-( I wish I did.
I think this is pretty much in line with how plenty of check matchers already 
work though, in particular unless(isInTemplateInstantiation()) is basically the 
same thing and is used in many places including this check.
So I wouldn't be particularly worried.

(Obviously from first principles you'd prefer to prune the subtree rather than 
match within it and then walk up the parent chain, but I don't think matchers 
supports this pattern well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: alexfh, sammccall.
aaron.ballman added subscribers: sammccall, alexfh.
aaron.ballman added a comment.

Thanks for the fix, can you also add a release note for it?

I'm a bit worried about using `hasAncestor()` for this; that has a tendency to 
do surprising things in addition to being expensive because it's very greedy. 
However, I can't see a situation in which it's going to be wrong, because the 
body of the requires expression is always unevaluated (so we really don't need 
to worry about nonsense like defining a lambda in the requires body, and then 
defining a class in the lambda, and having a redundant expression we care to 
diagnose in a member function of that class).

@alexfh or @sammccall -- do you see any concerns with this use of 
`hasAncestor()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122078: [clang-tidy] Ignore concepts in `misc-redundant-expression`

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron created this revision.
Izaron added reviewers: hokein, njames93, aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
Izaron requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The checker should ignore requirement expressions inside concept
definitions, because redundant expressions still make sense here

Fixes https://github.com/llvm/llvm-project/issues/54114


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122078

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s misc-redundant-expression -std=c++20 %t -- -- -fno-delayed-template-parsing
 
 typedef __INT64_TYPE__ I64;
 
@@ -807,3 +807,13 @@
 };
 
 } // namespace no_crash
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template 
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -10,6 +10,7 @@
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
@@ -438,6 +439,8 @@
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -831,6 +834,7 @@
 
   const auto BannedIntegerLiteral =
   integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -846,7 +850,8 @@
unless(hasType(realFloatingPointType())),
unless(hasEitherOperand(hasType(realFloatingPointType(,
unless(hasLHS(AnyLiteralExpr)),
-   unless(hasDescendant(BannedIntegerLiteral)))
+   unless(hasDescendant(BannedIntegerLiteral)),
+   unless(hasAncestor(BannedAncestor)))
.bind("binary")),
   this);
 
@@ -859,7 +864,8 @@
  unless(isInTemplateInstantiation()),
  unless(binaryOperatorIsInMacro()),
  // TODO: if the banned macros are themselves duplicated
- unless(hasDescendant(BannedIntegerLiteral)))
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -869,7 +875,8 @@
conditionalOperator(expressionsAreEquivalent(),
// Filter noisy false positives.
unless(conditionalOperatorIsInMacro()),
-   unless(isInTemplateInstantiation()))
+   unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("cond")),
   this);
 
@@ -882,7 +889,8 @@
 ">=", "&&", "||", "="),
parametersAreEquivalent(),
// Filter noisy false positives.
-   unless(isMacro()), unless(isInTemplateInstantiation()))
+   unless(isMacro()), unless(isInTemplateInstantiation()),
+   unless(hasAncestor(BannedAncestor)))
.bind("call")),
   this);
 
@@ -892,7 +900,8 @@
   hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
   nestedParametersAreEquivalent(), argumentCountIs(2),
   // Filter noisy false positives.
-  unless(isMacro()), unless(isInTemplateInstantiation()))
+  unless(isMacro()), unless(isInTemplateInstantiation()),
+  unless(hasAncestor(BannedAncestor)))
   .bind("nested-duplicates"),
   this);
 
@@ -909,7 +918,8 @@
binaryOperator(hasAnyOperatorName("|",