[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<BinaryOperator>(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null<BinaryOperator>(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