[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

Fri Mar 29 09:19:00 PDT 2019

hintonda added a comment.

In D59802#1447560 <https://reviews.llvm.org/D59802#1447560>, @aaron.ballman wrote:

> In D59802#1445566 <https://reviews.llvm.org/D59802#1445566>, @hintonda wrote:
>
> > I looked at the IR generated at `-O2`, and found that while `if (isa<X>(y))` is a modest win over `if (dyn_cast<X>(y)`,  `if (dyn_cast_or_null<X>(y))` generates exactly the same IR that `if(y && isa<X>(y))` does.  Also, if `y` is actually an expression that makes a function call, it's more expensive because it will make the call twice.
> >
> > So I don't seen any reason to replace `dyn_cast_or_null<>` in conditionals.
>
>
> Mostly because I think it's more clear with `isa<>` because that's really what it's checking. However, I could see not doing an automatic transform in the case where the expression argument is something expensive, like a function call. I could also see this as an impetus for adding `isa_or_null<>` as a separate commit.

My last patch only changes `if(y && isa<X>(y))` if `y` is a `CXXMemberCallExpr`, so I hope that addresses your concern.

================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+    diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
----------------
aaron.ballman wrote:
> This diagnostic doesn't tell the user what they've done wrong with the code or why this is a better choice.
Yes, but I'm not yet sure what it should say.  Was sorta hoping for a suggestion.  

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802