[clang-tidy] Handle member variables in readability-simplify-boolean-expr

[LegalizeAdulthood]

Summary

• Add readability-simplify-boolean-expr test cases for member variables

Fixes #40179

[LegalizeAdulthood]

Fix typo in id

[LegalizeAdulthood]

Fix typo in id

[LegalizeAdulthood]

No, really, fix typo in id

[JonasToth]

Thanks for the patch. Is this revision dependent on D56303 (or other way around)?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

476

The const for these locals in uncommon in clang-tidy, given its a value type. I am not strongly against them, but would prefer consistency.

test/clang-tidy/readability-simplify-bool-expr-members.cpp

5

I didn't see a test that utilized this struct, did I overlook it?

Implicit conversion to bool-cases would be interesting as well. Maybe implicit conversion to integral in general.

36

Would bitfields with a single bit be of interest as well?

161

For this case the logical inversion is simple an obviously <=, but what if the condition is more complex?

I would personally prefer !(i < 20) instead + complex logical expressions as additional test. They would be interesting for the if cases, too.

[LegalizeAdulthood]

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

476

I can undo the const.

test/clang-tidy/readability-simplify-bool-expr-members.cpp

5

Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove it.

Implicit conversion cases are covered by the other file. Here, I'm just testing the cases that interact with member variables because the matcher needs to use memberExpr() and not just declRefExpr()

36

That could be an enhancement, but it's not addressed by this patch.

161

If you have complex conditions, you can run the check repeatedly until you reach a fixed point.

Using !(x) instead of the reverse operator could be added as an enhancement, but it is not covered by this patch.

[JonasToth]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

476

Ok, that would be great.

test/clang-tidy/readability-simplify-bool-expr-members.cpp

5

Which other file? The other revision?

161

But the current version is the enhancement.
The complex condition, like i < 20 && i > 0 will be the common case, so the inversion (with parens) should always be used (!(i < 20 && i > 0)) and specializations can be done later.

[LegalizeAdulthood]

test/clang-tidy/readability-simplify-bool-expr-members.cpp

5

I based this new file off readability-simplify-bool-expr.cpp that is already there. It has all the main test cases for this check.

161

No, the current version is a bug fix. The simplification of boolean expressions was already there, but it didn't handle member variables properly.

You're asking for an additional enhancement about the way boolean expressions are simplified. That's fine for an additional enhancement of the check, but it is not the point of this patch.

The complex expression you're discussing in your comment is not changed by this change, nor is it changed by the existing check.

[LegalizeAdulthood]

Update from review comments

[JonasToth]

test/clang-tidy/readability-simplify-bool-expr-members.cpp

161

I see, then I did misunderstand these changes.

[JonasToth]

Did you check if the changes actually fix the problematic behaviour? Did you run it over any project and did the code-transformation break the code, false positives, ...?

Otherwise LGTM

[LegalizeAdulthood]

In D56323#1347450, @JonasToth wrote:

Did you check if the changes actually fix the problematic behaviour? Did you run it over any project and did the code-transformation break the code, false positives, ...?

Otherwise LGTM

The test cases that I added covered the behavior described in the bug report. I didn't run it over clang to see if it introduced any false positives, I can do that. However, when I originally wrote this check it was pretty well tested and I have confidence that I haven't regressed. However, it's still worth testing, so I will do that.

[LegalizeAdulthood]

Uh... I'm developing on Windows, which doesn't have a solution for generating compile_commands.json, so I can't easily test this on the entire clang codebase. I will test on a source file based on the original bug report.

[LegalizeAdulthood]

I managed to do some limited testing on the original source file from the bug report in lldb and verified that the correct fix was applied there. I also tried a few other files and verified no false positives.

[JonasToth]

In D56323#1347489, @LegalizeAdulthood wrote:

I managed to do some limited testing on the original source file from the bug report in lldb and verified that the correct fix was applied there. I also tried a few other files and verified no false positives.

Thanks. That is no issue, I will run this check locally over multiple projects once committed. If I find issues I will report back!

[JonasToth]

@LegalizeAdulthood your stuck on the commit right things right? If you want I can commit for you (maybe even later) as you said you are limited on time.

[LegalizeAdulthood]

In D56323#1350998, @JonasToth wrote:

@LegalizeAdulthood your stuck on the commit right things right? If you want I can commit for you (maybe even later) as you said you are limited on time.

Yes, please. I haven't figured out what the new commit procedure is or how I can submit. If you could submit this for me, that would help me out, thanks.

[JonasToth]

Thank you for the patch and sorry for the long delay.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

 const char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
 const char IfReturnsBoolId[] = "if-return";
 const char IfReturnsNotBoolId[] = "if-not-return";
 const char ThenLiteralId[] = "then-literal";
 const char IfAssignVariableId[] = "if-assign-lvalue";
 const char IfAssignLocId[] = "if-assign-loc";
 const char IfAssignBoolId[] = "if-assign";
 const char IfAssignNotBoolId[] = "if-assign-not";
-const char IfAssignObjId[] = "if-assign-obj";
+const char IfAssignVarId[] = "if-assign-var";
 const char CompoundReturnId[] = "compound-return";
 const char CompoundBoolId[] = "compound-bool";
 const char CompoundNotBoolId[] = "compound-bool-not";
 
 const char IfStmtId[] = "if";
 
 const char SimplifyOperatorDiagnostic[] =
     "redundant boolean literal supplied to boolean operator";
bool containsBoolLiteral(const Expr *E) {
   return false;
 }
 
 void SimplifyBooleanExprCheck::reportBinOp(
     const MatchFinder::MatchResult &Result, const BinaryOperator *Op) {
   const auto *LHS = Op->getLHS()->IgnoreParenImpCasts();
   const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
-  const CXXBoolLiteralExpr *Bool = nullptr;
+  const CXXBoolLiteralExpr *Bool;
   const Expr *Other = nullptr;
   if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)))
     Other = RHS;
   else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)))
     Other = LHS;
   else
     return;
 
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                               hasThen(returnsBool(Value, ThenLiteralId)),
                               hasElse(returnsBool(!Value)))
                            .bind(Id),
                        this);
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
-  auto SimpleThen = binaryOperator(
-      hasOperatorName("="),
-      hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId)))),
+  auto VarAssign = declRefExpr(hasDeclaration(decl().bind(IfAssignVarId)));
+  auto VarRef = declRefExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
+  auto MemAssign = memberExpr(hasDeclaration(decl().bind(IfAssignVarId)));
+  auto MemRef = memberExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
+  auto SimpleThen =
+      binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarAssign, MemAssign)),
                      hasLHS(expr().bind(IfAssignVariableId)),
                      hasRHS(cxxBoolLiteral(equals(Value)).bind(IfAssignLocId)));
   auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleThen)));
-  auto SimpleElse = binaryOperator(
-      hasOperatorName("="),
-      hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId)))),
+  auto SimpleElse =
+      binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarRef, MemRef)),
                      hasRHS(cxxBoolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
   if (ChainedConditionalAssignment)
-    Finder->addMatcher(
-        ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
-        this);
+    Finder->addMatcher(ifStmt(hasThen(Then), hasElse(Else)).bind(Id), this);
   else
-    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
-                              unless(hasParent(ifStmt())), hasThen(Then),
-                              hasElse(Else))
-                           .bind(Id),
-                       this);
+    Finder->addMatcher(
+        ifStmt(unless(hasParent(ifStmt())), hasThen(Then), hasElse(Else))
 }
 
 void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
                                                           bool Value,
                                                           StringRef Id) {
   Finder->addMatcher(
       compoundStmt(
           hasAnySubstatement(

test/clang-tidy/readability-simplify-bool-expr-members.cpp

+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b1\) {$}}
+    i = 5;
+  } else {
+    i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+    i = 7;
+  } else {
+    i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b3\) {$}}
+    i = 10;
+  } else {
+    i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+    i = 12;
+  } else {
+    i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b1\) {$}}
+    i = 14;
+  } else {
+    i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(true\) {$}}
+    i = 16;
+  } else {
+    i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b3\) {$}}
+    i = 18;
+  } else {
+    i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(true\) {$}}
+    i = 20;
+  } else {
+    i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(false\) {$}}
+    i = 22;
+  } else {
+    i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b2\) {$}}
+    i = 24;
+  } else {
+    i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(false\) {$}}
+    i = 26;
+  } else {
+    i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b4\) {$}}
+    i = 28;
+  } else {
+    i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b1\) {$}}
+    i = 30;
+  } else {
+    i = 31;
+  }
+  m_b2 = (i > 20);
+  if (true != m_b2) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+    i = 32;
+  } else {
+    i = 33;
+  }
+  m_b3 = (i > 20);
+  if (m_b3 != false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(m_b3\) {$}}
+    i = 34;
+  } else {
+    i = 35;
+  }
+  m_b4 = (i > 20);
+  if (m_b4 != true) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+    i = 36;
+  } else {
+    i = 37;
+  }
+}
+
+void S::simple_conditional_assignment_statements() {
+  if (m_i > 10)
+    m_b1 = true;
+  else
+    m_b1 = false;
+  bool bb = false;
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES: {{^  }}m_b1 = m_i > 10;{{$}}
+  // CHECK-FIXES: bool bb = false;
+
+  if (m_i > 20)
+    m_b2 = false;
+  else
+    m_b2 = true;
+  bool c2 = false;
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES: {{^  }}m_b2 = m_i <= 20;{{$}}
+  // CHECK-FIXES: bool c2 = false;
+
+  // Unchanged: different variables.
+  if (m_i > 12)
+    m_b1 = true;
+  else
+    m_b2 = false;
+
+  // Unchanged: no else statement.
+  if (m_i > 15)
+    m_b3 = true;
+
+  // Unchanged: not boolean assignment.
+  int j;
+  if (m_i > 17)
+    j = 10;
+  else
+    j = 20;
+
+  // Unchanged: different variables assigned.
+  int k = 0;
+  m_b4 = false;
+  if (m_i > 10)
+    m_b4 = true;
+  else
+    k = 10;
+}
+
+void S::complex_conditional_assignment_statements() {
+  if (m_i > 30) {
+    m_b1 = true;
+  } else {
+    m_b1 = false;
+  }
+  m_b1 = false;
+  // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES: {{^  }}m_b1 = m_i > 30;{{$}}
+  // CHECK-FIXES: m_b1 = false;
+
+  if (m_i > 40) {
+    m_b2 = false;
+  } else {
+    m_b2 = true;
+  }
+  m_b2 = false;
+  // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES: {{^  }}m_b2 = m_i <= 40;{{$}}
+  // CHECK-FIXES: m_b2 = false;
+}
+
+// Unchanged: chained return statements, but ChainedConditionalReturn not set.
+void S::chained_conditional_assignment() {
+  if (m_i < 0)
+    m_b1 = true;
+  else if (m_i < 10)
+    m_b1 = false;
+  else if (m_i > 20)
+    m_b1 = true;
+  else
+    m_b1 = false;
+}
+
+bool S::non_null_pointer_condition() {
+  if (m_p) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_p != nullptr;{{$}}
+
+bool S::null_pointer_condition() {
+  if (!m_p) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_p == nullptr;{{$}}
+
+bool S::negated_non_null_pointer_condition() {
+  if (m_p) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_p == nullptr;{{$}}
+
+bool S::negated_null_pointer_condition() {
+  if (!m_p) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_p != nullptr;{{$}}
+
+bool S::integer_not_zero() {
+  if (m_i) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return m_i == 0;{{$}}
+
+bool S::member_pointer_nullptr() {
+  if (m_m) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_m != nullptr;{{$}}
+
+bool S::integer_member_implicit_cast() {
+  if (m_a->m) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return m_a->m != 0;{{$}}
+
+bool operator!=(const A &, const A &) { return false; }
+bool S::expr_with_cleanups() {
+  if (m_ar != (A)m_ar)
+    return false;
+
+  return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: m_ar == (A)m_ar;{{$}}