[PATCH] D59802: [clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+    diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
----------------
hintonda wrote:
> aaron.ballman wrote:
> > hintonda wrote:
> > > 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.  
> > Do you have any evidence that this situation happens in practice? I kind of feel like this entire branch could be eliminated from this patch unless it actually catches problems that happen.
> Yes, here are a few from clang/lib -- let me know if you think it's worth it or not to keep this:
> 
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 305293
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
>   Message: method 'getAsTemplateDecl' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
>     Length: 93
>     Offset: 305293
>     ReplacementText: dyn_cast_or_null<TemplateTemplateParmDecl>(Name.getAsTemplateDecl())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 153442
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
>   Message: method 'getAsTemplateDecl' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
>     Length: 92
>     Offset: 153442
>     ReplacementText: dyn_cast_or_null<TypeAliasTemplateDecl>(Template.getAsTemplateDecl())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 97556
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
>   Message: method 'getMethodDecl' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
>     Length: 68
>     Offset: 97556
>     ReplacementText: dyn_cast_or_null<CXXConversionDecl>(MCE->getMethodDecl())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 301950
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
>   Message: method 'get' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
>     Length: 49
>     Offset: 301950
>     ReplacementText: dyn_cast_or_null<InitListExpr>(CurInit.get())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 14335
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
>   Message: method 'operator bool' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
>     Length: 57
>     Offset: 14335
>     ReplacementText: dyn_cast_or_null<CXXTryStmt>(B->getTerminator())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 15997
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
>   Message: method 'operator bool' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
>     Length: 55
>     Offset: 15997
>     ReplacementText: dyn_cast_or_null<CXXTryStmt>(B.getTerminator())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 9492
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>   Message: method 'sexpr' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>     Length: 39
>     Offset: 9492
>     ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 9572
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>   Message: method 'sexpr' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>     Length: 38
>     Offset: 9572
>     ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 9492
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>   Message: method 'sexpr' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>     Length: 39
>     Offset: 9492
>     ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
> - DiagnosticName: llvm-avoid-cast-in-conditional
>   FileOffset: 9572
>   FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>   Message: method 'sexpr' is called twice and could be expensive
>   Replacements:
>   - FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
>     Length: 38
>     Offset: 9572
>     ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())
> 
`/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:`

This one is a false positive and the fix is incorrect. The original code is:
```
    Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
      << (Name.getAsTemplateDecl() &&
          isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));
```
The rest look to be plausible true positives, but not a single case looks like there is much expense involved in the expression, and honestly, several of the replacement expressions would feel a bit "worse" to me, despite being equivalent. For instance, code like: `bool Foo = bar && isa<T>(bar);` reads more clearly to me than `bool Foo = dyn_cast_or_null<T>(bar);` because the former looks like a bool initializer while the latter looks like the user meant to write `T *Foo` instead of `bool Foo`.

I think this part of the check is contentious and I'd rather leave it out for now so that we can get the rest of the check in. I'd be especially curious to know whether the community would prefer to see us leave this situation alone, introduce `isa_or_null<T>()`, or use `dyn_cast_or_null<T>()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802