[PATCH] D59802: [clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals
Don Hinton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 3 19:39:55 PDT 2019
hintonda marked an inline comment as done.
hintonda 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:
> > > > 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>()`.
> How is that a false positive? In
>
> ```
> Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
> << (Name.getAsTemplateDecl() &&
> isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));
> ```
>
> `Name.getAsTemplateDecl()` is called twice. First to see if returned `nullptr` or not, then to see if the pointer satisfied `isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl())`.
>
> As for whether or not it's worth having, I'll defer to you. Most cases are relatively cheap, but it can be expensive. I'll go back and rerun it on all of clang+llvm and find the expensive ones if you're interested, but it takes a really long time to run on my laptop, so let me know what you'd prefer.
>
Btw, I'll draft a message to the list and ask about `isa_or_null`, et al, in the morning. I didn't want to add stuff like that in this patch, but would be happy to use it if I got the green light to add it in another.
Thanks again for the feedback -- especially on the matchers.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59802/new/
https://reviews.llvm.org/D59802
More information about the cfe-commits
mailing list