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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59802#1443515 <https://reviews.llvm.org/D59802#1443515>, @hintonda wrote:

> In D59802#1443451 <https://reviews.llvm.org/D59802#1443451>, @hintonda wrote:
>
> > In D59802#1443340 <https://reviews.llvm.org/D59802#1443340>, @aaron.ballman wrote:
> >
> > > Should this check also try to find this pattern:
> > >
> > >   if (dyn_cast<Frobble>(Bobble))
> > >     ...
> > >
> > >
> > > in order to recommend the proper replacement:
> > >
> > >   if (isa<Frobble>(Bobble))
> > >     ...
> > >
> > >
> > > I ask because the name `llvm-avoid-cast-in-conditional` sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into `cast<>` being used in a conditional).
> >
> >
> > Yes, I can add that, and provide a fix-it too.  Thanks...
>
>
> I did a quick grep and found a few of these, but also found the `_or_null<>` variety.  Guess they'll need to stay the same since `isa<>` can't handle nulls.


Would it make sense to transform `if (dyn_cast_or_null<T>(Obj))` into `if (Obj && isa<T>(Obj))`  or are there bad transformations from that?

> Btw, I also found the same pattern used for `while()`, so I'll add that too.  Here's a sample of the patterns I'm seeing:
> 
> ./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213:  while (dyn_cast<NullStmt>(last_stmt)) {

Hah, good catch!

> ./clang/lib/CodeGen/CodeGenModule.cpp:1390:  if (dyn_cast_or_null<NamedDecl>(D)) .     // <--- this one's okay

I think this could be expressed as `if (D && isa<NamedDecl>(D))`, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802