[clang-tidy][misc-redundant-expression] Support for CXXFoldExpr

[zinovy.nis]

Summary

The revision adds a support for CXXFoldExpr allowing handling cases like

template <class... Values>
struct Bar2 {
  static_assert((... && (sizeof(Values) > 0)) || (... && (sizeof(Values) > 0)));
  //  warning: both sides of operator are equivalent [misc-redundant-expression]
};

Also fixes the bug https://bugs.llvm.org/show_bug.cgi?id=44256

[zinovy.nis]

Fix formatting.

[zinovy.nis]

Fix and simplify the condition.

[njames93]

This fix has expanded from preventing the crash to adding support for CXXFoldExpr to misc-redundant-expression. Maybe rename the revision to explain that better.
If that is the case then you may as well add a test case showing a redundant fold expression

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

74–75

What happens when *LeftIter is null, but *RightIter isn't or vice versa, That case should also return false.

[zinovy.nis]

• Fix test.
• Simplify the code.

[zinovy.nis]

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

18

I had to move this up as no warnings were generated in the original test code despite -fno-delayed-template-parsing.

[zinovy.nis]

This comment was removed by zinovy.nis.

[zinovy.nis]

@njames93, @aaron.ballman do you have any other comments?

[njames93]

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

31

See other comment but if you change this to use == instead of || you can move this all back to the bottom simplifying the changes in this PR.

172

This is why you aren't getting warnings issued when the test case is at the bottom. You have defined a || operator at global namespace level which I'm guessing leads to ambiguity when checking the static_assert operator in your test case - It'll have a dependent type.
Simple fix is to change the static_assert to use say operator== as that isn't defined globally.

[zinovy.nis]

Simplify test.

[zinovy.nis]

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

172

It helped, thanks!

[aaron.ballman]

LGTM, but please wait a bit in case @njames93 has any final thoughts.

[njames93]

LGTM, nothing else to add.

[njames93]

How are you landing these changes, because the commit message isn't lining up with the PR name?

[zinovy.nis]

In D80896#2076735, @njames93 wrote:

How are you landing these changes, because the commit message isn't lining up with the PR name?

It was my fault: I hadn't changed a title in my local branch and pushed it with git.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
 
   // Compare classes.
   if (Left->getStmtClass() != Right->getStmtClass())
     return false;
 
   // Compare children.
   Expr::const_child_iterator LeftIter = Left->child_begin();
   Expr::const_child_iterator RightIter = Right->child_begin();
   while (LeftIter != Left->child_end() && RightIter != Right->child_end()) {
-    if (!areEquivalentExpr(dyn_cast<Expr>(*LeftIter),
-                           dyn_cast<Expr>(*RightIter)))
+    if (!areEquivalentExpr(dyn_cast_or_null<Expr>(*LeftIter),
+                           dyn_cast_or_null<Expr>(*RightIter)))
       return false;
     ++LeftIter;
     ++RightIter;
   }
   if (LeftIter != Left->child_end() || RightIter != Right->child_end())
     return false;
 
   // Perform extra checks.
return areEquivalentNameSpecifier(
         cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
         cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
   case Stmt::DeclRefExprClass:
     return cast<DeclRefExpr>(Left)->getDecl() ==
            cast<DeclRefExpr>(Right)->getDecl();
   case Stmt::MemberExprClass:
     return cast<MemberExpr>(Left)->getMemberDecl() ==
            cast<MemberExpr>(Right)->getMemberDecl();
+  case Stmt::CXXFoldExprClass:
+    return cast<CXXFoldExpr>(Left)->getOperator() ==
+           cast<CXXFoldExpr>(Right)->getOperator();
   case Stmt::CXXFunctionalCastExprClass:
   case Stmt::CStyleCastExprClass:
     return cast<ExplicitCastExpr>(Left)->getTypeAsWritten() ==
            cast<ExplicitCastExpr>(Right)->getTypeAsWritten();
   case Stmt::CallExprClass:
   case Stmt::ImplicitCastExprClass:
   case Stmt::ArraySubscriptExprClass:
     return true;

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

 
 extern Point P1;
 extern Point P2;
 
 extern int foo(int x);
 extern int bar(int x);
 extern int bat(int x, int y);
 
 int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
struct MyStruct {
   bool operator>=(const MyStruct& rhs) const { return this->x >= rhs.x; } // not modifing
   bool operator<=(MyStruct& rhs) const { return this->x <= rhs.x; }
   bool operator&&(const MyStruct& rhs){ this->x++; return this->x && rhs.x; }
 } Q;
 
 bool operator!=(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } // not modifing
 bool operator<(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x < rhs.x; } // not modifing
 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; }

 bool operator<(const Foo&, const Foo&);
 template <class T>
 struct Bar {
   static const Foo &GetFoo();
   static bool Test(const T & maybe_foo, const Foo& foo) {
     return foo < GetFoo() && foo < maybe_foo;
   }
 };
-}
+
+template <class... Values>
+struct Bar2 {
+  static_assert((... && (sizeof(Values) > 0)) == (... && (sizeof(Values) > 0)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: both sides of operator are equivalent [misc-redundant-expression]
+};
+} // namespace no_crash