[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D73775#1875921 , @alexeyr wrote:

> @aaron.ballman I just noticed this in 
> https://mlir.llvm.org/getting_started/Contributing/
>
> > Note: if you haven’t used Arcanist to send your patch for review, 
> > committers don’t have access to your preferred identity for commit 
> > messages. Make sure to communicate it to them through available channels or 
> > use the git sign-off functionality to make your identity visible in the 
> > commit message.
>
> Can you please make sure the email is listed as `romanov.alex...@huawei.com` 
> when committing (I've just set it to primary, so it may be automatic?)


Absolutely -- thank you for clarifying what email address you wanted used. I've 
commit on your behalf in 5e7d0ebf735a8b70f92acd1f91c7c45423e611cc 



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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

@aaron.ballman I just noticed this in 
https://mlir.llvm.org/getting_started/Contributing/

> Note: if you haven’t used Arcanist to send your patch for review, committers 
> don’t have access to your preferred identity for commit messages. Make sure 
> to communicate it to them through available channels or use the git sign-off 
> functionality to make your identity visible in the commit message.

Can you please make sure the email is listed as `romanov.alex...@huawei.com` 
when committing?


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

Yes, I do. Thank you (and @Eugene.Zelenko) for your help making this better!


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Still LGTM, thank you! If you need me to commit this on your behalf, I'm happy 
to do so, just let me know. It'll have to wait until next week, though (unless 
someone else does it first).


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 244370.
alexeyr added a comment.

Actually updated.


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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: operator has equivalent nested operands
+  if ((X ^ Y) ^ (Y ^ X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: operator has equivalent nested operands
+
   return 0;
 }
 
@@ -106,6 +113,7 @@
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1 return 1;
@@ -163,6 +171,15 @@
 bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
 bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
 
+struct MyStruct1 {
+  bool x;
+  MyStruct1(bool x) : x(x) {};
+  operator bool() { return x; }
+};
+
+MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; }
+MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; }
+
 bool TestOverloadedOperator(MyStruct& S) {
   if (S == Q) return false;
 
@@ -180,6 +197,15 @@
   if (S >= S) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 
+  MyStruct1 U(false);
+  MyStruct1 V(true);
+
+  // valid because the operator is not const
+  if ((U || V) || U) return true;
+
+  if (U && V && U && V) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has equivalent nested operands
+
   return true;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,132 @@
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr,
+OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+OverloadedOperatorKind OpKind) {
+  if (const auto *BinOp = checkOpKind(Part, OpKind)) {
+const std::pair Operands = getOperands(BinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  }
+
+  AllOperands.push_back(Part);
+  return false;
+}
+
+template 
+static bool hasSameOperatorParent(const Expr *TheExpr,
+  OverloadedOperatorKind OpKind,
+  ASTContext ) {
+  // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes
+  const DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for (ast_type_traits::DynTypedNode DynParent : Parents) {
+ 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 244369.
alexeyr added a comment.

@aaron.ballman Updated.


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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: operator has equivalent nested operands
+  if ((X ^ Y) ^ (Y ^ X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: operator has equivalent nested operands
+
   return 0;
 }
 
@@ -106,6 +113,7 @@
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1 return 1;
@@ -163,6 +171,15 @@
 bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
 bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
 
+struct MyStruct1 {
+  bool x;
+  MyStruct1(bool x) : x(x) {};
+  operator bool() { return x; }
+};
+
+MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; }
+MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; }
+
 bool TestOverloadedOperator(MyStruct& S) {
   if (S == Q) return false;
 
@@ -180,6 +197,15 @@
   if (S >= S) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 
+  MyStruct1 U(false);
+  MyStruct1 V(true);
+
+  // valid because the operator is not const
+  if ((U || V) || U) return true;
+
+  if (U && V && U && V) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has equivalent nested operands
+
   return true;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,133 @@
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr,
+OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+OverloadedOperatorKind OpKind) {
+  const auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool hasSameOperatorParent(const Expr *TheExpr,
+  OverloadedOperatorKind OpKind,
+  ASTContext ) {
+  // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes
+  const DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[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.

LGTM aside from a minor nit.




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:355
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);

No `else` after a `return`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:206
+
+  if (U && V && U && V) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has 
equivalent nested operands

I think that this is reasonable behavior, however, once the user overloads the 
operator there's no way to know whether calling that operator has other side 
effects that make this not actually be equivalent to `U && V`. That said, 
unless this happens an awful lot in practice, this still seems like a 
reasonable heuristic.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

Ping. Are any other changes needed?


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

In D73775#1859167 , @aaron.ballman 
wrote:

> Huh, this does seem like it may be a bug in clang-tidy. I would have expected 
> `ClangTidyDiagnosticConsumer::forwardDiagnostic()` to do this work.


From my debugging attempt it seems not to be called (it's only supposed to be 
"If there is an external diagnostics engine, like in the ClangTidyPluginAction 
case"). Reported at https://bugs.llvm.org/show_bug.cgi?id=44799.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 242572.
alexeyr added a comment.

Lower-case warning messages


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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: operator has equivalent nested operands
+  if ((X ^ Y) ^ (Y ^ X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: operator has equivalent nested operands
+
   return 0;
 }
 
@@ -106,6 +113,7 @@
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1 return 1;
@@ -163,6 +171,15 @@
 bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
 bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
 
+struct MyStruct1 {
+  bool x;
+  MyStruct1(bool x) : x(x) {};
+  operator bool() { return x; }
+};
+
+MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; }
+MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; }
+
 bool TestOverloadedOperator(MyStruct& S) {
   if (S == Q) return false;
 
@@ -180,6 +197,15 @@
   if (S >= S) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 
+  MyStruct1 U(false);
+  MyStruct1 V(true);
+
+  // valid because the operator is not const
+  if ((U || V) || U) return true;
+
+  if (U && V && U && V) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: overloaded operator has equivalent nested operands
+
   return true;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,133 @@
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr,
+OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+OverloadedOperatorKind OpKind) {
+  const auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool hasSameOperatorParent(const Expr *TheExpr,
+  OverloadedOperatorKind OpKind,
+  ASTContext ) {
+  // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes
+  const DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D73775#1859156 , @alexeyr wrote:

> In D73775#1856869 , @aaron.ballman 
> wrote:
>
> > In D73775#1856848 , @alexeyr wrote:
> >
> > > In D73775#1856765 , 
> > > @aaron.ballman wrote:
> > >
> > > > In D73775#1851553 , @alexeyr 
> > > > wrote:
> > > >
> > > > > Also I am not sure why, but the ranges added to the diagnostic in 
> > > > > lines 1222-1226 don't show up in the message.
> > > >
> > > >
> > > > Do you mean that there are no squiggly underlines under the range, or 
> > > > something else? Passing in a range to the diagnostics engine gives it 
> > > > something to put an underline under, as in what's under the -12 in: 
> > > > https://godbolt.org/z/GeQzYg
> > >
> > >
> > > Yes, exactly. I expect to see the underlines, but don't; only the `^` in 
> > > the location provided to `diag` call.
> >
> >
> > The only time I've seen that happen is when the range is invalid. I'm 
> > guessing you'll have to step through the diagnostics code to see what's 
> > going on.
>
>
> After stepping through diagnostics code, I... don't understand how it is 
> supposed to work. `ClangTidyDiagnosticConsumer` is
>
> > A diagnostic consumer that turns each Diagnostic into a 
> > SourceManager-independent ClangTidyError.
>
> And it adds the //hints// to the `ClangTidyError` here 
> :
>
>   for (const auto  : Hints) { ... }
>
>
> But not the ranges, and I don't see any place where it does or even how they 
> can be inserted into a `ClangTidyError` in the first place!


Huh, this does seem like it may be a bug in clang-tidy. I would have expected 
`ClangTidyDiagnosticConsumer::forwardDiagnostic()` to do this work.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

In D73775#1856869 , @aaron.ballman 
wrote:

> In D73775#1856848 , @alexeyr wrote:
>
> > In D73775#1856765 , @aaron.ballman 
> > wrote:
> >
> > > In D73775#1851553 , @alexeyr 
> > > wrote:
> > >
> > > > Also I am not sure why, but the ranges added to the diagnostic in lines 
> > > > 1222-1226 don't show up in the message.
> > >
> > >
> > > Do you mean that there are no squiggly underlines under the range, or 
> > > something else? Passing in a range to the diagnostics engine gives it 
> > > something to put an underline under, as in what's under the -12 in: 
> > > https://godbolt.org/z/GeQzYg
> >
> >
> > Yes, exactly. I expect to see the underlines, but don't; only the `^` in 
> > the location provided to `diag` call.
>
>
> The only time I've seen that happen is when the range is invalid. I'm 
> guessing you'll have to step through the diagnostics code to see what's going 
> on.


After stepping through diagnostics code, I... don't understand how it is 
supposed to work. `ClangTidyDiagnosticConsumer` is

> A diagnostic consumer that turns each Diagnostic into a 
> SourceManager-independent ClangTidyError.

And it adds the //hints// to the `ClangTidyError` here 
:

  for (const auto  : Hints) { ... }

But not the ranges, and I don't see any place where it does or even how they 
can be inserted into a `ClangTidyError` in the first place!


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D73775#1856848 , @alexeyr wrote:

> In D73775#1856765 , @aaron.ballman 
> wrote:
>
> > In D73775#1851553 , @alexeyr wrote:
> >
> > > Also I am not sure why, but the ranges added to the diagnostic in lines 
> > > 1222-1226 don't show up in the message.
> >
> >
> > Do you mean that there are no squiggly underlines under the range, or 
> > something else? Passing in a range to the diagnostics engine gives it 
> > something to put an underline under, as in what's under the -12 in: 
> > https://godbolt.org/z/GeQzYg
>
>
> Yes, exactly. I expect to see the underlines, but don't; only the `^` in the 
> location provided to `diag` call.


The only time I've seen that happen is when the range is invalid. I'm guessing 
you'll have to step through the diagnostics code to see what's going on.




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:1242
+StringRef Message =
+Call ? "Overloaded operator has equivalent nested operands"
+ : "Operator has equivalent nested operands";

Diagnostics in clang tidy are not capitalized, so this should be `overloaded 
operator` and `operator`.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

In D73775#1856765 , @aaron.ballman 
wrote:

> In D73775#1851553 , @alexeyr wrote:
>
> > Also I am not sure why, but the ranges added to the diagnostic in lines 
> > 1222-1226 don't show up in the message.
>
>
> Do you mean that there are no squiggly underlines under the range, or 
> something else? Passing in a range to the diagnostics engine gives it 
> something to put an underline under, as in what's under the -12 in: 
> https://godbolt.org/z/GeQzYg


Yes, exactly. I expect to see the underlines, but don't; only the `^` in the 
location provided to `diag` call.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D73775#1851553 , @alexeyr wrote:

> Also I am not sure why, but the ranges added to the diagnostic in lines 
> 1222-1226 don't show up in the message.


Do you mean that there are no squiggly underlines under the range, or something 
else? Passing in a range to the diagnostics engine gives it something to put an 
underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr marked an inline comment as done.
alexeyr added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 

aaron.ballman wrote:
> alexeyr wrote:
> > aaron.ballman wrote:
> > > What do you think about the following?
> > > ```
> > > bool foo(int&);
> > > bool bar();
> > > 
> > > int i;
> > > if (foo(i) && bar() && foo(i)) return 1;
> > > ```
> > > I think that this should not be warned on (under the assumption that the 
> > > reference variable can be modified by the call and thus may or may not be 
> > > duplicate), but didn't see a test covering it.
> > > 
> > > It also brings up an interesting question about what to do if those were 
> > > non-const pointers rather than references, because the data being pointed 
> > > to could be modified as well.
> > > 
> > > (If you think this should be done separately from this review, that's 
> > > totally fine by me, it looks like it would be an issue with the original 
> > > code as well.)
> > > 
> > > One thing that is missing from this review are tests for the overloaded 
> > > operator functionality.
> > This is actually handled correctly, by the same logic as `(X && X++ && X)`, 
> > so I don't think it needs a separate test. The drawback is that:
> > 
> > 1. it's too conservative, `X && bar() && X` isn't warned on either, because 
> > I don't know a way to check that `bar()` doesn't have side effects //on 
> > `X`// and so just test `HasSideEffects` 
> > (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).
> > 
> > 2. the original code does have the same issue and I didn't fix it, so 
> > `foo(X) && foo(X)` and `X++ && X++` do get a warning. 
> > 
> > I'll add overloaded operator tests.
> Okay, that seems reasonable to me, thank you!
I've added the tests (which uncovered a problem not limited to overloaded 
operators; I needed to skip uninteresting nodes when looking at parents as 
well).


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr marked an inline comment as done.
alexeyr added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 

alexeyr wrote:
> aaron.ballman wrote:
> > alexeyr wrote:
> > > aaron.ballman wrote:
> > > > What do you think about the following?
> > > > ```
> > > > bool foo(int&);
> > > > bool bar();
> > > > 
> > > > int i;
> > > > if (foo(i) && bar() && foo(i)) return 1;
> > > > ```
> > > > I think that this should not be warned on (under the assumption that 
> > > > the reference variable can be modified by the call and thus may or may 
> > > > not be duplicate), but didn't see a test covering it.
> > > > 
> > > > It also brings up an interesting question about what to do if those 
> > > > were non-const pointers rather than references, because the data being 
> > > > pointed to could be modified as well.
> > > > 
> > > > (If you think this should be done separately from this review, that's 
> > > > totally fine by me, it looks like it would be an issue with the 
> > > > original code as well.)
> > > > 
> > > > One thing that is missing from this review are tests for the overloaded 
> > > > operator functionality.
> > > This is actually handled correctly, by the same logic as `(X && X++ && 
> > > X)`, so I don't think it needs a separate test. The drawback is that:
> > > 
> > > 1. it's too conservative, `X && bar() && X` isn't warned on either, 
> > > because I don't know a way to check that `bar()` doesn't have side 
> > > effects //on `X`// and so just test `HasSideEffects` 
> > > (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).
> > > 
> > > 2. the original code does have the same issue and I didn't fix it, so 
> > > `foo(X) && foo(X)` and `X++ && X++` do get a warning. 
> > > 
> > > I'll add overloaded operator tests.
> > Okay, that seems reasonable to me, thank you!
> I've added the tests (which uncovered a problem not limited to overloaded 
> operators; I needed to skip uninteresting nodes when looking at parents as 
> well).
Do you have any insight on https://reviews.llvm.org/D73775#1851553?


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 242266.
alexeyr added a comment.

Added tests for overloaded operators and fixed parent check logic.


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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Operator has equivalent nested operands
+  if ((X ^ Y) ^ (Y ^ X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Operator has equivalent nested operands
+
   return 0;
 }
 
@@ -106,6 +113,7 @@
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1 return 1;
@@ -163,6 +171,15 @@
 bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
 bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
 
+struct MyStruct1 {
+  bool x;
+  MyStruct1(bool x) : x(x) {};
+  operator bool() { return x; }
+};
+
+MyStruct1 operator&&(const MyStruct1& lhs, const MyStruct1& rhs) { return lhs.x && rhs.x; }
+MyStruct1 operator||(MyStruct1& lhs, MyStruct1& rhs) { return lhs.x && rhs.x; }
+
 bool TestOverloadedOperator(MyStruct& S) {
   if (S == Q) return false;
 
@@ -180,6 +197,15 @@
   if (S >= S) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 
+  MyStruct1 U(false);
+  MyStruct1 V(true);
+
+  // valid because the operator is not const
+  if ((U || V) || U) return true;
+
+  if (U && V && U && V) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: Overloaded operator has equivalent nested operands
+
   return true;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,133 @@
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr,
+OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+OverloadedOperatorKind OpKind) {
+  const auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool hasSameOperatorParent(const Expr *TheExpr,
+  OverloadedOperatorKind OpKind,
+  ASTContext ) {
+  // IgnoreParenImpCasts logic in reverse: skip surrounding uninteresting nodes
+  const ASTContext::DynTypedNodeList Parents = 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 

alexeyr wrote:
> aaron.ballman wrote:
> > What do you think about the following?
> > ```
> > bool foo(int&);
> > bool bar();
> > 
> > int i;
> > if (foo(i) && bar() && foo(i)) return 1;
> > ```
> > I think that this should not be warned on (under the assumption that the 
> > reference variable can be modified by the call and thus may or may not be 
> > duplicate), but didn't see a test covering it.
> > 
> > It also brings up an interesting question about what to do if those were 
> > non-const pointers rather than references, because the data being pointed 
> > to could be modified as well.
> > 
> > (If you think this should be done separately from this review, that's 
> > totally fine by me, it looks like it would be an issue with the original 
> > code as well.)
> > 
> > One thing that is missing from this review are tests for the overloaded 
> > operator functionality.
> This is actually handled correctly, by the same logic as `(X && X++ && X)`, 
> so I don't think it needs a separate test. The drawback is that:
> 
> 1. it's too conservative, `X && bar() && X` isn't warned on either, because I 
> don't know a way to check that `bar()` doesn't have side effects //on `X`// 
> and so just test `HasSideEffects` 
> (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).
> 
> 2. the original code does have the same issue and I didn't fix it, so `foo(X) 
> && foo(X)` and `X++ && X++` do get a warning. 
> 
> I'll add overloaded operator tests.
Okay, that seems reasonable to me, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr marked an inline comment as done.
alexeyr added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 

aaron.ballman wrote:
> What do you think about the following?
> ```
> bool foo(int&);
> bool bar();
> 
> int i;
> if (foo(i) && bar() && foo(i)) return 1;
> ```
> I think that this should not be warned on (under the assumption that the 
> reference variable can be modified by the call and thus may or may not be 
> duplicate), but didn't see a test covering it.
> 
> It also brings up an interesting question about what to do if those were 
> non-const pointers rather than references, because the data being pointed to 
> could be modified as well.
> 
> (If you think this should be done separately from this review, that's totally 
> fine by me, it looks like it would be an issue with the original code as 
> well.)
> 
> One thing that is missing from this review are tests for the overloaded 
> operator functionality.
This is actually handled correctly, by the same logic as `(X && X++ && X)`, so 
I don't think it needs a separate test. The drawback is that:

1. it's too conservative, `X && bar() && X` isn't warned on either, because I 
don't know a way to check that `bar()` doesn't have side effects //on `X`// and 
so just test `HasSideEffects` 
(https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).

2. the original code does have the same issue and I didn't fix it, so `foo(X) 
&& foo(X)` and `X++ && X++` do get a warning. 

I'll add overloaded operator tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 

What do you think about the following?
```
bool foo(int&);
bool bar();

int i;
if (foo(i) && bar() && foo(i)) return 1;
```
I think that this should not be warned on (under the assumption that the 
reference variable can be modified by the call and thus may or may not be 
duplicate), but didn't see a test covering it.

It also brings up an interesting question about what to do if those were 
non-const pointers rather than references, because the data being pointed to 
could be modified as well.

(If you think this should be done separately from this review, that's totally 
fine by me, it looks like it would be an issue with the original code as well.)

One thing that is missing from this review are tests for the overloaded 
operator functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr marked 11 inline comments as done.
alexeyr added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:363
+  ASTContext ) {
+  const auto OpKind = getOp(TheExpr);
+  // if there are no nested operators of the same kind, it's handled by

alexeyr wrote:
> Eugene.Zelenko wrote:
> > Please don't use auto when type is not spelled explicitly or iterator.
> In this case the type will depend on `TExpr`: either `BinaryOperator::Opcode` 
> or `OverloadedOperatorKind`. I could make it a template parameter (but it 
> won't be deducible) or convert `OverloadedOperatorKind` to `Opcode`. Any 
> preference?
Converted both to `OverloadedOperatorKind` (because I have a use for `OO_None`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 242000.
alexeyr added a comment.

Followed Eugene Zelenko's comments and fixed a false positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,11 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Operator has equivalent nested operands
+
   return 0;
 }
 
@@ -106,6 +111,7 @@
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1 return 1;
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,114 @@
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr, OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+OverloadedOperatorKind OpKind) {
+  const auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool
+markDuplicateOperands(const TExpr *TheExpr,
+  ast_matchers::internal::BoundNodesTreeBuilder *Builder,
+  ASTContext ) {
+  const OverloadedOperatorKind OpKind = getOp(TheExpr);
+  if (OpKind == OO_None)
+return false;
+  // if there are no nested operators of the same kind, it's handled by
+  // operands/parametersAreEquivalent
+  const std::pair Operands = getOperands(TheExpr);
+  if (!(checkOpKind(Operands.first, OpKind) ||
+checkOpKind(Operands.second, OpKind)))
+return false;
+
+  // if parent is the same kind of operator, it's handled by a previous call to
+  // markDuplicateOperands
+  const ASTContext::DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+if (checkOpKind(Parent.get(), OpKind))
+  return false;
+  }
+
+  SmallVector AllOperands;
+  if (collectOperands(Operands.first, AllOperands, OpKind))
+return false;
+  if (collectOperands(Operands.second, AllOperands, OpKind))
+return false;
+  size_t NumOperands = AllOperands.size();
+  llvm::SmallBitVector Duplicates(NumOperands);
+  for (size_t I = 0; I < NumOperands; I++) {
+if (Duplicates[I])
+  continue;
+bool FoundDuplicates = false;
+
+for (size_t J = I + 1; J < NumOperands; J++) {
+  if (AllOperands[J]->HasSideEffects(Context))
+break;
+
+  if 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:363
+  ASTContext ) {
+  const auto OpKind = getOp(TheExpr);
+  // if there are no nested operators of the same kind, it's handled by

Eugene.Zelenko wrote:
> Please don't use auto when type is not spelled explicitly or iterator.
In this case the type will depend on `TExpr`: either `BinaryOperator::Opcode` 
or `OverloadedOperatorKind`. I could make it a template parameter (but it won't 
be deducible) or convert `OverloadedOperatorKind` to `Opcode`. Any preference?



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:375
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+if (checkOpKind(Parent.get(), OpKind)) {
+  return false;

Eugene.Zelenko wrote:
> Please elide braces.
Should braces be elided from `for` as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:335
+return AsTExpr;
+  else
+return nullptr;

Please don;e use else after return.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:345
+TOp OpKind) {
+  auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {

Could it be const auto *?



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:347
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))

Please don't use auto when type is not spelled explicitly or iterator.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:352
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);

Please elide braces.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:363
+  ASTContext ) {
+  const auto OpKind = getOp(TheExpr);
+  // if there are no nested operators of the same kind, it's handled by

Please don't use auto when type is not spelled explicitly or iterator.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:366
+  // operands/parametersAreEquivalent
+  const auto Operands = getOperands(TheExpr);
+  if (!(checkOpKind(Operands.first, OpKind) ||

Please don't use auto when type is not spelled explicitly or iterator.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:375
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+if (checkOpKind(Parent.get(), OpKind)) {
+  return false;

Please elide braces.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:402
+
+if (FoundDuplicates) {
+  Builder->setBinding(

Please elide braces.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:1223
+for (const auto  : Result.Nodes.getMap()) {
+  if (StringRef(KeyValue.first).startswith("duplicate")) {
+Diag << KeyValue.second.getSourceRange();

Please elide braces.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

Also I am not sure why, but the ranges added to the diagnostic in lines 
1222-1226 don't show up in the message.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr updated this revision to Diff 241696.
alexeyr added a comment.

Fixed test broken by git clang-format


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

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X)
+return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Operator has equivalent nested operands
+  if (X || (Y || X))
+return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Operator has equivalent nested operands
+
   return 0;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,109 @@
   }
 }
 
+// to use in the template below
+static BinaryOperator::Opcode getOp(const BinaryOperator *Op) {
+  return Op->getOpcode();
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr, TOp OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+  else
+return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+TOp OpKind) {
+  auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool
+markDuplicateOperands(const TExpr *TheExpr,
+  ast_matchers::internal::BoundNodesTreeBuilder *Builder,
+  ASTContext ) {
+  const auto OpKind = getOp(TheExpr);
+  // if there are no nested operators of the same kind, it's handled by
+  // operands/parametersAreEquivalent
+  const auto Operands = getOperands(TheExpr);
+  if (!(checkOpKind(Operands.first, OpKind) ||
+checkOpKind(Operands.second, OpKind)))
+return false;
+
+  // if parent is the same kind of operator, it's handled by a previous call to
+  // markDuplicateOperands
+  const ASTContext::DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+if (checkOpKind(Parent.get(), OpKind)) {
+  return false;
+}
+  }
+
+  SmallVector AllOperands;
+  if (collectOperands(Operands.first, AllOperands, OpKind))
+return false;
+  if (collectOperands(Operands.second, AllOperands, OpKind))
+return false;
+  size_t NumOperands = AllOperands.size();
+  llvm::SmallBitVector Duplicates(NumOperands);
+  for (size_t I = 0; I < NumOperands; I++) {
+if (Duplicates[I])
+  continue;
+bool FoundDuplicates = false;
+
+for (size_t J = I + 1; J < NumOperands; J++) {
+  if (areEquivalentExpr(AllOperands[I], AllOperands[J])) {
+FoundDuplicates = true;
+Duplicates.set(J);
+Builder->setBinding(
+SmallString<11>(llvm::formatv("duplicate{0}", J)),
+ast_type_traits::DynTypedNode::create(*AllOperands[J]));
+  }
+}
+
+if (FoundDuplicates) {
+  Builder->setBinding(
+  SmallString<11>(llvm::formatv("duplicate{0}", I)),
+  ast_type_traits::DynTypedNode::create(*AllOperands[I]));
+}
+  }
+
+  return Duplicates.any();
+}
+
 AST_MATCHER(Expr, 

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr added a comment.

Note: during testing I found that `++X && ++X` is a false positive, my current 
implementation also introduces `X && ++X && X` which I'll try to fix.


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

https://reviews.llvm.org/D73775



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

[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[Alexey Romanov via Phabricator via cfe-commits]

alexeyr created this revision.
alexeyr added reviewers: alexfh, etienneb, hgabii.
alexeyr added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

readability-redundant-expression could detect expressions where a logical or 
bitwise operator had equivalent LHS and RHS, but not ones where equivalent 
operands were separated by more operands.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73775

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
@@ -81,6 +81,13 @@
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X)
+return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Operator has equivalent nested operands
+  if (X || (Y || X))
+return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Operator has equivalent nested operands
+
   return 0;
 }
 
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
@@ -18,7 +18,9 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include 
 #include 
 #include 
@@ -304,6 +306,109 @@
   }
 }
 
+// to use in the template below
+static BinaryOperator::Opcode getOp(const BinaryOperator *Op) {
+  return Op->getOpcode();
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  return Op->getOperator();
+}
+
+static std::pair
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+  Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+  Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template 
+static const TExpr *checkOpKind(const Expr *TheExpr, TOp OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+return AsTExpr;
+  else
+return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template 
+static bool collectOperands(const Expr *Part,
+SmallVector ,
+TOp OpKind) {
+  auto *PartAsBinOp = checkOpKind(Part, OpKind);
+  if (PartAsBinOp) {
+auto Operands = getOperands(PartAsBinOp);
+if (areEquivalentExpr(Operands.first, Operands.second))
+  return true;
+return collectOperands(Operands.first, AllOperands, OpKind) ||
+   collectOperands(Operands.second, AllOperands, OpKind);
+  } else {
+AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template 
+static bool
+markDuplicateOperands(const TExpr *TheExpr,
+  ast_matchers::internal::BoundNodesTreeBuilder *Builder,
+  ASTContext ) {
+  const auto OpKind = getOp(TheExpr);
+  // if there are no nested operators of the same kind, it's handled by
+  // operands/parametersAreEquivalent
+  const auto Operands = getOperands(TheExpr);
+  if (!(checkOpKind(Operands.first, OpKind) ||
+checkOpKind(Operands.second, OpKind)))
+return false;
+
+  // if parent is the same kind of operator, it's handled by a previous call to
+  // markDuplicateOperands
+  const ASTContext::DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+if (checkOpKind(Parent.get(), OpKind)) {
+  return false;
+}
+  }
+
+  SmallVector AllOperands;
+  if (collectOperands(Operands.first, AllOperands, OpKind))
+return false;
+  if (collectOperands(Operands.second, AllOperands, OpKind))
+return false;
+  size_t NumOperands = AllOperands.size();
+  llvm::SmallBitVector Duplicates(NumOperands);
+  for (size_t I = 0; I < NumOperands; I++) {
+if (Duplicates[I])
+  continue;
+bool FoundDuplicates = false;
+
+for (size_t J = I + 1; J < NumOperands; J++) {
+  if (areEquivalentExpr(AllOperands[I], AllOperands[J])) {
+FoundDuplicates = true;
+Duplicates.set(J);
+Builder->setBinding(
+SmallString<11>(llvm::formatv("duplicate{0}", J)),
+ast_type_traits::DynTypedNode::create(*AllOperands[J]));
+