[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdf5335a36d3d: [clang-tidy] readability-simplify-boolean-expr 
detects negated literals (authored by njames93).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

Files:
  clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
@@ -98,6 +98,46 @@
   // CHECK-FIXES-NEXT: {{^  i = 11;$}}
 }
 
+void if_with_negated_bool_condition() {
+  int i = 10;
+  if (!true) {
+i = 11;
+  } else {
+i = 12;
+  }
+  i = 13;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  int i = 10;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^i = 12;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 13;$}}
+
+  i = 14;
+  if (!false) {
+i = 15;
+  } else {
+i = 16;
+  }
+  i = 17;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  i = 14;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^i = 15;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 17;$}}
+
+  i = 18;
+  if (!true) {
+i = 19;
+  }
+  i = 20;
+  // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  i = 18;$}}
+  // CHECK-FIXES-NEXT: {{^  $}}
+  // CHECK-FIXES-NEXT: {{^  i = 20;$}}
+}
+
 void operator_equals() {
   int i = 0;
   bool b1 = (i > 2);
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -51,11 +51,11 @@
 
   void
   replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult ,
-   const CXXBoolLiteralExpr *BoolLiteral);
+   const Expr *BoolLiteral);
 
   void
   replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult ,
-   const CXXBoolLiteralExpr *FalseConditionRemoved);
+   const Expr *FalseConditionRemoved);
 
   void
   replaceWithCondition(const ast_matchers::MatchFinder::MatchResult ,
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -58,16 +58,28 @@
 const char SimplifyConditionalReturnDiagnostic[] =
 "redundant boolean literal in conditional return statement";
 
-const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult ,
- StringRef Id) {
-  const auto *Literal = Result.Nodes.getNodeAs(Id);
-  return (Literal && Literal->getBeginLoc().isMacroID()) ? nullptr : Literal;
+const Expr *getBoolLiteral(const MatchFinder::MatchResult ,
+   StringRef Id) {
+  if (const Expr *Literal = Result.Nodes.getNodeAs(Id))
+return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))
+  return Negated->getBeginLoc().isMacroID() ? nullptr : Negated;
+  }
+  return nullptr;
+}
+
+internal::BindableMatcher literalOrNegatedBool(bool Value) {
+  return expr(anyOf(cxxBoolLiteral(equals(Value)),
+unaryOperator(hasUnaryOperand(ignoringParenImpCasts(
+  cxxBoolLiteral(equals(!Value,
+  hasOperatorName("!";
 }
 
 internal::Matcher returnsBool(bool Value, StringRef Id = "ignored") {
-  auto SimpleReturnsBool =
-  returnStmt(has(cxxBoolLiteral(equals(Value)).bind(Id)))
-  .bind("returns-bool");
+  auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
+   .bind("returns-bool");
   return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
@@ -269,16 +281,25 @@
   return asBool(getText(Result, *E), NeedsStaticCast);
 }
 
-const CXXBoolLiteralExpr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+const Expr *stmtReturnsBool(const ReturnStmt 

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[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, thank you for the fix!




Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))

njames93 wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > I'd like to see this handled recursively instead so that we properly 
> > > > handle more esoteric logic (but it still shows up from time to time) 
> > > > like `!!foo`: 
> > > > https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21=Search
> > > > 
> > > > This is a case where I don't think the simplification should happen -- 
> > > > e.g., the code is usually subtly incorrect when converted to remove the 
> > > > `!!`. It's less clear to me whether the same is true for something like 
> > > > `!!!` being converted to `!`, but that's not a case I'm really worried 
> > > > about either.
> > > As this is only looking for boolean literals that shouldn't matter
> > > `!!true` is identical to `true`.
> > > I get putting `!!` in front of expressions that aren't boolean has an 
> > > effect, but we aren't looking for that in this case.
> > Oh, derp, I saw "SimplifyBooleanExprCheck" and assumed it was simplifying 
> > arbitrary boolean expressions, not just literals. I am now far less worried 
> > about supporting this case, it's up to you if you want to do the extra 
> > work. Thank you for clarifying and sorry for my think-o!
> Personally I don't think the extra complexity is worth it. Usages of `!true`, 
> I expect are rather low but likely show up in a few places, however I doubt 
> anyone would use `!!true`. 
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

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

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > I'd like to see this handled recursively instead so that we properly 
> > > handle more esoteric logic (but it still shows up from time to time) like 
> > > `!!foo`: 
> > > https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21=Search
> > > 
> > > This is a case where I don't think the simplification should happen -- 
> > > e.g., the code is usually subtly incorrect when converted to remove the 
> > > `!!`. It's less clear to me whether the same is true for something like 
> > > `!!!` being converted to `!`, but that's not a case I'm really worried 
> > > about either.
> > As this is only looking for boolean literals that shouldn't matter
> > `!!true` is identical to `true`.
> > I get putting `!!` in front of expressions that aren't boolean has an 
> > effect, but we aren't looking for that in this case.
> Oh, derp, I saw "SimplifyBooleanExprCheck" and assumed it was simplifying 
> arbitrary boolean expressions, not just literals. I am now far less worried 
> about supporting this case, it's up to you if you want to do the extra work. 
> Thank you for clarifying and sorry for my think-o!
Personally I don't think the extra complexity is worth it. Usages of `!true`, I 
expect are rather low but likely show up in a few places, however I doubt 
anyone would use `!!true`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

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

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))

njames93 wrote:
> aaron.ballman wrote:
> > I'd like to see this handled recursively instead so that we properly handle 
> > more esoteric logic (but it still shows up from time to time) like `!!foo`: 
> > https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21=Search
> > 
> > This is a case where I don't think the simplification should happen -- 
> > e.g., the code is usually subtly incorrect when converted to remove the 
> > `!!`. It's less clear to me whether the same is true for something like 
> > `!!!` being converted to `!`, but that's not a case I'm really worried 
> > about either.
> As this is only looking for boolean literals that shouldn't matter
> `!!true` is identical to `true`.
> I get putting `!!` in front of expressions that aren't boolean has an effect, 
> but we aren't looking for that in this case.
Oh, derp, I saw "SimplifyBooleanExprCheck" and assumed it was simplifying 
arbitrary boolean expressions, not just literals. I am now far less worried 
about supporting this case, it's up to you if you want to do the extra work. 
Thank you for clarifying and sorry for my think-o!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

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

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))

aaron.ballman wrote:
> I'd like to see this handled recursively instead so that we properly handle 
> more esoteric logic (but it still shows up from time to time) like `!!foo`: 
> https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21=Search
> 
> This is a case where I don't think the simplification should happen -- e.g., 
> the code is usually subtly incorrect when converted to remove the `!!`. It's 
> less clear to me whether the same is true for something like `!!!` being 
> converted to `!`, but that's not a case I'm really worried about either.
As this is only looking for boolean literals that shouldn't matter
`!!true` is identical to `true`.
I get putting `!!` in front of expressions that aren't boolean has an effect, 
but we aren't looking for that in this case.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:74
+internal::BindableMatcher literalOrNegatedBool(bool Value) {
+  return expr(anyOf(cxxBoolLiteral(equals(Value)),
+unaryOperator(hasUnaryOperand(ignoringParenImpCasts(

aaron.ballman wrote:
> Oof, but handling it here may be tricky...
A custom matcher could be made for this that could traverse a `UnaryOperator`



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:289
   }
+  if (const auto *Unary = dyn_cast(Ret->getRetValue())) {
+if (Unary->getOpcode() == UO_LNot) {

aaron.ballman wrote:
> Same here.
This could be easily handled with some recursion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

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

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))

I'd like to see this handled recursively instead so that we properly handle 
more esoteric logic (but it still shows up from time to time) like `!!foo`: 
https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21=Search

This is a case where I don't think the simplification should happen -- e.g., 
the code is usually subtly incorrect when converted to remove the `!!`. It's 
less clear to me whether the same is true for something like `!!!` being 
converted to `!`, but that's not a case I'm really worried about either.



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:74
+internal::BindableMatcher literalOrNegatedBool(bool Value) {
+  return expr(anyOf(cxxBoolLiteral(equals(Value)),
+unaryOperator(hasUnaryOperand(ignoringParenImpCasts(

Oof, but handling it here may be tricky...



Comment at: 
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:289
   }
+  if (const auto *Unary = dyn_cast(Ret->getRetValue())) {
+if (Unary->getOpcode() == UO_LNot) {

Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86176

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

[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, gribozavr2, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
njames93 requested review of this revision.

Adds support for detecting cases like `if (!true) ...`.
Addresses readability-simplify-boolean-expr not detected for negated boolean 
literals. 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86176

Files:
  clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
@@ -98,6 +98,46 @@
   // CHECK-FIXES-NEXT: {{^  i = 11;$}}
 }
 
+void if_with_negated_bool_condition() {
+  int i = 10;
+  if (!true) {
+i = 11;
+  } else {
+i = 12;
+  }
+  i = 13;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  int i = 10;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^i = 12;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 13;$}}
+
+  i = 14;
+  if (!false) {
+i = 15;
+  } else {
+i = 16;
+  }
+  i = 17;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  i = 14;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^i = 15;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 17;$}}
+
+  i = 18;
+  if (!true) {
+i = 19;
+  }
+  i = 20;
+  // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:  {{^  i = 18;$}}
+  // CHECK-FIXES-NEXT: {{^  $}}
+  // CHECK-FIXES-NEXT: {{^  i = 20;$}}
+}
+
 void operator_equals() {
   int i = 0;
   bool b1 = (i > 2);
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -51,11 +51,11 @@
 
   void
   replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult ,
-   const CXXBoolLiteralExpr *BoolLiteral);
+   const Expr *BoolLiteral);
 
   void
   replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult ,
-   const CXXBoolLiteralExpr *FalseConditionRemoved);
+   const Expr *FalseConditionRemoved);
 
   void
   replaceWithCondition(const ast_matchers::MatchFinder::MatchResult ,
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -58,16 +58,28 @@
 const char SimplifyConditionalReturnDiagnostic[] =
 "redundant boolean literal in conditional return statement";
 
-const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult ,
- StringRef Id) {
-  const auto *Literal = Result.Nodes.getNodeAs(Id);
-  return (Literal && Literal->getBeginLoc().isMacroID()) ? nullptr : Literal;
+const Expr *getBoolLiteral(const MatchFinder::MatchResult ,
+   StringRef Id) {
+  if (const Expr *Literal = Result.Nodes.getNodeAs(Id))
+return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
+  if (const auto *Negated = Result.Nodes.getNodeAs(Id)) {
+if (Negated->getOpcode() == UO_LNot &&
+isa(Negated->getSubExpr()))
+  return Negated->getBeginLoc().isMacroID() ? nullptr : Negated;
+  }
+  return nullptr;
+}
+
+internal::BindableMatcher literalOrNegatedBool(bool Value) {
+  return expr(anyOf(cxxBoolLiteral(equals(Value)),
+unaryOperator(hasUnaryOperand(ignoringParenImpCasts(
+  cxxBoolLiteral(equals(!Value,
+  hasOperatorName("!";
 }
 
 internal::Matcher returnsBool(bool Value, StringRef Id = "ignored") {
-  auto SimpleReturnsBool =
-  returnStmt(has(cxxBoolLiteral(equals(Value)).bind(Id)))
-  .bind("returns-bool");
+  auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
+   .bind("returns-bool");
   return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
@@ -269,16 +281,25 @@
   return asBool(getText(Result, *E),