[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > zinovy.nis wrote:
> > > > aaron.ballman wrote:
> > > > > zinovy.nis wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > There's not a whole lot of context for FileCheck to determine if 
> > > > > > > it's been correctly applied or not (same below) -- for instance, 
> > > > > > > won't this pass even if no changes are applied because FileCheck 
> > > > > > > is still going to find `isSet` in the file?
> > > > > > Thanks. Fixed.
> > > > > Maybe it's just early in the morning for me, but... I was expecting 
> > > > > the transformation to be:
> > > > > ```
> > > > > if (RetT::Test(isSet).Ok() && isSet) {
> > > > >   if (RetT::Test(isSet).Ok() && isSet) {
> > > > >   }
> > > > > }
> > > > > ```
> > > > > turns into
> > > > > ```
> > > > > if (RetT::Test(isSet).Ok() && isSet) {
> > > > > }
> > > > > ```
> > > > > Why does it remove the `&& isSet` instead? That seems like it's 
> > > > > changing the logic here from `if (true && false)` to `if (true)`.
> > > > IMO it's correct.
> > > > `isSet` cannot change its value between `if`s while 
> > > > `RetT::Test(isSet).Ok()` can.
> > > > So we don't need to re-evaluate `isSet` and need to re-evaluate 
> > > > `RetT::Test(isSet).Ok()` only.
> > > > 
> > > > 
> > > > 
> > > > > That seems like it's changing the logic here from if (true && false) 
> > > > > to if (true).
> > > > 
> > > > 
> > > > As I understand only the second `if` is transformed.
> > > Does this only trigger as a redundant branch condition if the definition 
> > > of `RetT::Test()` is available? Because `Test()` takes a `bool&` so it 
> > > sure seems like `isSet` could be modified between the branches if the 
> > > definition isn't found because it's being passed as a non-const reference 
> > > to `Test()`.
> > 1) 
> > > if the definition of RetT::Test() is available?
> > 
> > Removing the body from RetT::Test changes nothing.
> > 
> > 2) Turning `RetT Test(bool &_isSet)` -> `RetT Test(bool _isSet)` also 
> > changes nothing.
> > 
> > 
> > 
> > 
> Given the following four ways of declaring `Test()`:
> ```
> static RetT Test(bool &_isSet); // #1
> static RetT Test(bool _isSet); // #2
> static RetT Test(const bool &_isSet); // #3
> static RetT Test(bool &_isSet) { return 0; } // #4
> ```
> I would expect #2 and #3 to behave the same way -- the `isSet` argument could 
> never be modified by calling `Test()` and so the second `Test(isSet) && 
> isSet` is partially redundant and the second `if` statement can drop the ` && 
> isSet`. (Without dropping the `Test(isSet)` because the call could still 
> modify some global somewhere, etc.)
> 
> I would expect #1 to not modify the second `if` statement at all because 
> there's no way of knowing whether `Test(isSet) && isSet` is the same between 
> the first `if` statement and the second one (because the second call to 
> `Test(isSet)` may modify `isSet` in a way the caller can observe).
> 
> Ideally, #4 would be a case where we could remove the entire second `if` 
> statement because we can identify that not only does `isSet` not get modified 
> despite being passed by non-const reference, we can see that the `Test()` 
> function doesn't modify any state at all. However, this seems like it would 
> require data flow analysis and so I think it makes sense to treat #4 the same 
> as #1.
> 
> That said, I just realized the check isn't being very careful with reference 
> parameters in the first place: https://godbolt.org/z/P1aP3W, so your changes 
> aren't introducing a new problem, they just happened to highlight an existing 
> issue.
Please look at https://reviews.llvm.org/D91495 - there's a fix for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

In D91037#2391915 , @aaron.ballman 
wrote:

> LGTM!

Thanks! I'll see what can be done to deal with it in a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4364539b3a4c: [clang-tidy] Fix crash in 
bugprone-redundant-branch-condition on… (authored by zinovy.nis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if (RetT::Test(isSet).Ok() ) {
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if ((RetT::Test(isSet).Ok() )) {
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreParenImpCasts());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if (RetT::Test(isSet).Ok() ) {
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if ((RetT::Test(isSet).Ok() )) {
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

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




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > zinovy.nis wrote:
> > > > > aaron.ballman wrote:
> > > > > > There's not a whole lot of context for FileCheck to determine if 
> > > > > > it's been correctly applied or not (same below) -- for instance, 
> > > > > > won't this pass even if no changes are applied because FileCheck is 
> > > > > > still going to find `isSet` in the file?
> > > > > Thanks. Fixed.
> > > > Maybe it's just early in the morning for me, but... I was expecting the 
> > > > transformation to be:
> > > > ```
> > > > if (RetT::Test(isSet).Ok() && isSet) {
> > > >   if (RetT::Test(isSet).Ok() && isSet) {
> > > >   }
> > > > }
> > > > ```
> > > > turns into
> > > > ```
> > > > if (RetT::Test(isSet).Ok() && isSet) {
> > > > }
> > > > ```
> > > > Why does it remove the `&& isSet` instead? That seems like it's 
> > > > changing the logic here from `if (true && false)` to `if (true)`.
> > > IMO it's correct.
> > > `isSet` cannot change its value between `if`s while 
> > > `RetT::Test(isSet).Ok()` can.
> > > So we don't need to re-evaluate `isSet` and need to re-evaluate 
> > > `RetT::Test(isSet).Ok()` only.
> > > 
> > > 
> > > 
> > > > That seems like it's changing the logic here from if (true && false) to 
> > > > if (true).
> > > 
> > > 
> > > As I understand only the second `if` is transformed.
> > Does this only trigger as a redundant branch condition if the definition of 
> > `RetT::Test()` is available? Because `Test()` takes a `bool&` so it sure 
> > seems like `isSet` could be modified between the branches if the definition 
> > isn't found because it's being passed as a non-const reference to `Test()`.
> 1) 
> > if the definition of RetT::Test() is available?
> 
> Removing the body from RetT::Test changes nothing.
> 
> 2) Turning `RetT Test(bool &_isSet)` -> `RetT Test(bool _isSet)` also changes 
> nothing.
> 
> 
> 
> 
Given the following four ways of declaring `Test()`:
```
static RetT Test(bool &_isSet); // #1
static RetT Test(bool _isSet); // #2
static RetT Test(const bool &_isSet); // #3
static RetT Test(bool &_isSet) { return 0; } // #4
```
I would expect #2 and #3 to behave the same way -- the `isSet` argument could 
never be modified by calling `Test()` and so the second `Test(isSet) && isSet` 
is partially redundant and the second `if` statement can drop the ` && isSet`. 
(Without dropping the `Test(isSet)` because the call could still modify some 
global somewhere, etc.)

I would expect #1 to not modify the second `if` statement at all because 
there's no way of knowing whether `Test(isSet) && isSet` is the same between 
the first `if` statement and the second one (because the second call to 
`Test(isSet)` may modify `isSet` in a way the caller can observe).

Ideally, #4 would be a case where we could remove the entire second `if` 
statement because we can identify that not only does `isSet` not get modified 
despite being passed by non-const reference, we can see that the `Test()` 
function doesn't modify any state at all. However, this seems like it would 
require data flow analysis and so I think it makes sense to treat #4 the same 
as #1.

That said, I just realized the check isn't being very careful with reference 
parameters in the first place: https://godbolt.org/z/P1aP3W, so your changes 
aren't introducing a new problem, they just happened to highlight an existing 
issue.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > zinovy.nis wrote:
> > > > aaron.ballman wrote:
> > > > > There's not a whole lot of context for FileCheck to determine if it's 
> > > > > been correctly applied or not (same below) -- for instance, won't 
> > > > > this pass even if no changes are applied because FileCheck is still 
> > > > > going to find `isSet` in the file?
> > > > Thanks. Fixed.
> > > Maybe it's just early in the morning for me, but... I was expecting the 
> > > transformation to be:
> > > ```
> > > if (RetT::Test(isSet).Ok() && isSet) {
> > >   if (RetT::Test(isSet).Ok() && isSet) {
> > >   }
> > > }
> > > ```
> > > turns into
> > > ```
> > > if (RetT::Test(isSet).Ok() && isSet) {
> > > }
> > > ```
> > > Why does it remove the `&& isSet` instead? That seems like it's changing 
> > > the logic here from `if (true && false)` to `if (true)`.
> > IMO it's correct.
> > `isSet` cannot change its value between `if`s while 
> > `RetT::Test(isSet).Ok()` can.
> > So we don't need to re-evaluate `isSet` and need to re-evaluate 
> > `RetT::Test(isSet).Ok()` only.
> > 
> > 
> > 
> > > That seems like it's changing the logic here from if (true && false) to 
> > > if (true).
> > 
> > 
> > As I understand only the second `if` is transformed.
> Does this only trigger as a redundant branch condition if the definition of 
> `RetT::Test()` is available? Because `Test()` takes a `bool&` so it sure 
> seems like `isSet` could be modified between the branches if the definition 
> isn't found because it's being passed as a non-const reference to `Test()`.
1) 
> if the definition of RetT::Test() is available?

Removing the body from RetT::Test changes nothing.

2) Turning `RetT Test(bool &_isSet)` -> `RetT Test(bool _isSet)` also changes 
nothing.






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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > There's not a whole lot of context for FileCheck to determine if it's 
> > > > been correctly applied or not (same below) -- for instance, won't this 
> > > > pass even if no changes are applied because FileCheck is still going to 
> > > > find `isSet` in the file?
> > > Thanks. Fixed.
> > Maybe it's just early in the morning for me, but... I was expecting the 
> > transformation to be:
> > ```
> > if (RetT::Test(isSet).Ok() && isSet) {
> >   if (RetT::Test(isSet).Ok() && isSet) {
> >   }
> > }
> > ```
> > turns into
> > ```
> > if (RetT::Test(isSet).Ok() && isSet) {
> > }
> > ```
> > Why does it remove the `&& isSet` instead? That seems like it's changing 
> > the logic here from `if (true && false)` to `if (true)`.
> IMO it's correct.
> `isSet` cannot change its value between `if`s while `RetT::Test(isSet).Ok()` 
> can.
> So we don't need to re-evaluate `isSet` and need to re-evaluate 
> `RetT::Test(isSet).Ok()` only.
> 
> 
> 
> > That seems like it's changing the logic here from if (true && false) to if 
> > (true).
> 
> 
> As I understand only the second `if` is transformed.
Does this only trigger as a redundant branch condition if the definition of 
`RetT::Test()` is available? Because `Test()` takes a `bool&` so it sure seems 
like `isSet` could be modified between the branches if the definition isn't 
found because it's being passed as a non-const reference to `Test()`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > There's not a whole lot of context for FileCheck to determine if it's 
> > > been correctly applied or not (same below) -- for instance, won't this 
> > > pass even if no changes are applied because FileCheck is still going to 
> > > find `isSet` in the file?
> > Thanks. Fixed.
> Maybe it's just early in the morning for me, but... I was expecting the 
> transformation to be:
> ```
> if (RetT::Test(isSet).Ok() && isSet) {
>   if (RetT::Test(isSet).Ok() && isSet) {
>   }
> }
> ```
> turns into
> ```
> if (RetT::Test(isSet).Ok() && isSet) {
> }
> ```
> Why does it remove the `&& isSet` instead? That seems like it's changing the 
> logic here from `if (true && false)` to `if (true)`.
IMO it's correct.
`isSet` cannot change its value between `if`s while `RetT::Test(isSet).Ok()` 
can.
So we don't need to re-evaluate `isSet` and need to re-evaluate 
`RetT::Test(isSet).Ok()` only.



> That seems like it's changing the logic here from if (true && false) to if 
> (true).


As I understand only the second `if` is transformed.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

zinovy.nis wrote:
> aaron.ballman wrote:
> > There's not a whole lot of context for FileCheck to determine if it's been 
> > correctly applied or not (same below) -- for instance, won't this pass even 
> > if no changes are applied because FileCheck is still going to find `isSet` 
> > in the file?
> Thanks. Fixed.
Maybe it's just early in the morning for me, but... I was expecting the 
transformation to be:
```
if (RetT::Test(isSet).Ok() && isSet) {
  if (RetT::Test(isSet).Ok() && isSet) {
  }
}
```
turns into
```
if (RetT::Test(isSet).Ok() && isSet) {
}
```
Why does it remove the `&& isSet` instead? That seems like it's changing the 
logic here from `if (true && false)` to `if (true)`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

aaron.ballman wrote:
> There's not a whole lot of context for FileCheck to determine if it's been 
> correctly applied or not (same below) -- for instance, won't this pass even 
> if no changes are applied because FileCheck is still going to find `isSet` in 
> the file?
Thanks. Fixed.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304719.
zinovy.nis added a comment.

Fixed fix-it section.


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if (RetT::Test(isSet).Ok() ) {
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if ((RetT::Test(isSet).Ok() )) {
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreParenImpCasts());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if (RetT::Test(isSet).Ok() ) {
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: if ((RetT::Test(isSet).Ok() )) {
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}

There's not a whole lot of context for FileCheck to determine if it's been 
correctly applied or not (same below) -- for instance, won't this pass even if 
no changes are applied because FileCheck is still going to find `isSet` in the 
file?


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:45
   ifStmt(
   hasCondition(ignoringParenImpCasts(anyOf(
   declRefExpr(hasDeclaration(ImmutableVar)),

njames93 wrote:
> Just noticed, as this is ignoring Parenthesis and implicit nodes/casts, maybe 
> we should also be ignoring those when getting the condition in the check
> `InnerIf->getCond()->IgnoreParenImpCasts()`
> I reckon if that change isnt made this could fail on
> ```lang=c++
> if (IsSet){
>   if ((OtherCond && IsSet))
> ;
> }
> ```
You are right, thanks! I fixed it.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304561.
zinovy.nis added a comment.

Use `IgnoreParenImpCasts`


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreParenImpCasts());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,34 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}
+  }
+  if (isSet) {
+if ((RetT::Test(isSet).Ok() && isSet)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{isSet}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D91037#2387377 , @njames93 wrote:

> Taking a step back, rather than worrying about if its an `ExprWithCleanups`. 
> Shouldn't we just get the condition removing all implicit nodes.
>
>   const Expr* Cond = InnerIf->getCond()->ignoreImplicit();
>
> This has to be simpler and will likely future proof this against any other 
> implicit nodes potentially added in future that can appear between the 
> Condition and the binary operator.

Good call!


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:45
   ifStmt(
   hasCondition(ignoringParenImpCasts(anyOf(
   declRefExpr(hasDeclaration(ImmutableVar)),

Just noticed, as this is ignoring Parenthesis and implicit nodes/casts, maybe 
we should also be ignoring those when getting the condition in the check
`InnerIf->getCond()->IgnoreParenImpCasts()`
I reckon if that change isnt made this could fail on
```lang=c++
if (IsSet){
  if ((OtherCond && IsSet))
;
}
```


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1092
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}

This line isn't really doing anything. `CHECK-FIXES` just runs FileCheck on the 
whole file after clang tidy has applied any fixes. You need to explicitly say 
what you are expecting it to be. 


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added a comment.

In D91037#2387377 , @njames93 wrote:

> Taking a step back, rather than worrying about if its an `ExprWithCleanups`. 
> Shouldn't we just get the condition removing all implicit nodes.
>
>   const Expr* Cond = InnerIf->getCond()->ignoreImplicit();
>
> This has to be simpler and will likely future proof this against any other 
> implicit nodes potentially added in future that can appear between the 
> Condition and the binary operator.

Nice! Thanks. Changed to use `IgnoreImplicit()`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:145
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());

aaron.ballman wrote:
> The old code used to assert that `CondOp` was a `BinaryOperator` but the new 
> code means that `CondOp` can be null -- should you add an `assert` in to 
> ensure we have a valid `CondOp` still or will that assert trigger on some 
> constructs?
Reverted to `cast`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304400.
zinovy.nis added a comment.

Simplified to use `IgnoreImplicit()`.


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,28 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreImplicit());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreImplicit());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,28 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreImplicit());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreImplicit());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Taking a step back, rather than worrying about if its an `ExprWithCleanups`. 
Shouldn't we just get the condition removing all implicit nodes.

  const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other 
implicit nodes potentially added in future that can appear between the 
Condition and the binary operator.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:145
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());

The old code used to assert that `CondOp` was a `BinaryOperator` but the new 
code means that `CondOp` can be null -- should you add an `assert` in to ensure 
we have a valid `CondOp` still or will that assert trigger on some constructs?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1077
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {

Can remove the newline here.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > zinovy.nis wrote:
> > > > aaron.ballman wrote:
> > > > > Under what circumstances does `getCond()` return `nullptr`?
> > > > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading 
> > > > the original `dyn_cast` to crash. That what this bug is about.
> > > > 
> > > That's what I figured, but you changed `dyn_cast<>` to be 
> > > `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't 
> > > return null so the `dyn_cast<>` was correct.
> > `dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
> > But `getCond()` can return (and it does so in the test case) 
> > `ExprWithCleanups` which is not a subclass of `BinaryExpression`.
> > That's why I use `_or_null` version of `dyn_cast`.
> > dyn_cast asserts unless getCond() returns BinaryExpression, right?
> 
> Nope.
> 
> `cast<>` asserts if the cast cannot be completed or if the cast input is null.
> `dyn_cast<>` returns null if the cast cannot be completed and asserts if the 
> cast input is null.
> There are `_or_null` variants of each which will accept a null input without 
> asserting.
Yes, you are right, I definitely need a vacation :-)
I confused `dyn_cast` with `cast` 
I've uploaded a fix.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304280.
zinovy.nis added a comment.

dyn_cast_or_null -> dyn_cast


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -83,6 +83,12 @@
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
   const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast(InnerIf->getCond()))
+  BinOpCond = dyn_cast(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast(InnerIf->getCond());
+if (!CondOp)
+  if (const auto *ExprWithCleanupsCond =
+  dyn_cast(InnerIf->getCond()))
+CondOp = dyn_cast(
+ExprWithCleanupsCond->getSubExpr());
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -83,6 +83,12 @@
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
   const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast(InnerIf->getCond()))
+  BinOpCond = dyn_cast(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast(InnerIf->getCond());
+if (!CondOp)
+

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > Under what circumstances does `getCond()` return `nullptr`?
> > > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> > > original `dyn_cast` to crash. That what this bug is about.
> > > 
> > That's what I figured, but you changed `dyn_cast<>` to be 
> > `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't return 
> > null so the `dyn_cast<>` was correct.
> `dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
> But `getCond()` can return (and it does so in the test case) 
> `ExprWithCleanups` which is not a subclass of `BinaryExpression`.
> That's why I use `_or_null` version of `dyn_cast`.
> dyn_cast asserts unless getCond() returns BinaryExpression, right?

Nope.

`cast<>` asserts if the cast cannot be completed or if the cast input is null.
`dyn_cast<>` returns null if the cast cannot be completed and asserts if the 
cast input is null.
There are `_or_null` variants of each which will accept a null input without 
asserting.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > Under what circumstances does `getCond()` return `nullptr`?
> > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> > original `dyn_cast` to crash. That what this bug is about.
> > 
> That's what I figured, but you changed `dyn_cast<>` to be 
> `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't return 
> null so the `dyn_cast<>` was correct.
`dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
But `getCond()` can return (and it does so in the test case) `ExprWithCleanups` 
which is not a subclass of `BinaryExpression`.
That's why I use `_or_null` version of `dyn_cast`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

zinovy.nis wrote:
> aaron.ballman wrote:
> > Under what circumstances does `getCond()` return `nullptr`?
> `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> original `dyn_cast` to crash. That what this bug is about.
> 
That's what I figured, but you changed `dyn_cast<>` to be `dyn_cast_or_null<>` 
and that seems incorrect -- `getCond()` doesn't return null so the `dyn_cast<>` 
was correct.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done and 2 inline comments as not done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> Under what circumstances does `getCond()` return `nullptr`?
`getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
original `dyn_cast` to crash. That what this bug is about.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:90
+  BinOpCond = dyn_cast_or_null(
+  *ExprWithCleanupsCond->children().begin());
+

aaron.ballman wrote:
> You can call `ExprWithCleanupsCond->getSubExpr()` to do this more cleanly -- 
> but similar question here as above -- what circumstances lead to a null sub 
> expression?
Thanks, fixed to use `getSubExpr()`.




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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304242.
zinovy.nis marked 2 inline comments as done.
zinovy.nis added a comment.

*iterator -> getSubExpr()


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast_or_null(InnerIf->getCond());
+if (!CondOp)
+  if (const auto *ExprWithCleanupsCond =
+  dyn_cast_or_null(InnerIf->getCond()))
+CondOp = dyn_cast_or_null(
+ExprWithCleanupsCond->getSubExpr());
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

Under what circumstances does `getCond()` return `nullptr`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:90
+  BinOpCond = dyn_cast_or_null(
+  *ExprWithCleanupsCond->children().begin());
+

You can call `ExprWithCleanupsCond->getSubExpr()` to do this more cleanly -- 
but similar question here as above -- what circumstances lead to a null sub 
expression?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:138
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast_or_null(InnerIf->getCond());
+if (!CondOp)

Same feedback here as above.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 303750.
zinovy.nis edited the summary of this revision.
zinovy.nis added a comment.

auto -> const auto.


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  *ExprWithCleanupsCond->children().begin());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast_or_null(InnerIf->getCond());
+if (!CondOp)
+  if (const auto *ExprWithCleanupsCond =
+  dyn_cast_or_null(InnerIf->getCond()))
+CondOp = dyn_cast_or_null(
+*ExprWithCleanupsCond->children().begin());
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  *ExprWithCleanupsCond->children().begin());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

In D91037#2381606 , @njames93 wrote:

> Think this has an incorrect name, seems to have something to do with 
> `bugprone-redundant-branch-condition`

Oops, thanks to autofill. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91037

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