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

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

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

> 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?


Sure, that sounds reasonable.  I only see about 12 cases across all repos, so it isn't that common.  Whereas the idiom you present is used quite often.

I haven't looked yet, but wouldn't the use of `isa_or_null<>` be more efficient in cases like this?

./clang/lib/Sema/AnalysisBasedWarnings.cpp:401:          if (B->getTerminator() && isa<CXXTryStmt>(B->getTerminator()))
./clang/lib/AST/Expr.cpp:2734:    if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))

Granted, there aren't a lot of these.

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