[PATCH] D59802: [clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 4 04:57:07 PDT 2019
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:
> 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.
> How is that a false positive?
Sorry! You are correct, I was being imprecise. I meant that the fix here is definitely bad and it demonstrates one of the things I was concerned about for the check. I also meant that the diagnostic firing here is suggesting that this code is wrong when in fact, naively conforming to the check would make the code incorrect. One possible solution to this is for the check to also include `static_cast<bool>(<expr>)`, but I'm not convinced that's great either.
> 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.
I went through all of the places in the list you posted above and did not see a single case where this would make a lot of difference in terms of performance. In every case it's a function call to a simple accessor, effectively.
================
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:
> hintonda wrote:
> > 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.
> > How is that a false positive?
>
> Sorry! You are correct, I was being imprecise. I meant that the fix here is definitely bad and it demonstrates one of the things I was concerned about for the check. I also meant that the diagnostic firing here is suggesting that this code is wrong when in fact, naively conforming to the check would make the code incorrect. One possible solution to this is for the check to also include `static_cast<bool>(<expr>)`, but I'm not convinced that's great either.
>
> > 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.
>
> I went through all of the places in the list you posted above and did not see a single case where this would make a lot of difference in terms of performance. In every case it's a function call to a simple accessor, effectively.
> 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.
Yup, agreed! I think the best approach right now is to see if `isa_or_null<>` gets some traction before moving forward with this part of the patch. You can either stall this patch until that discussion looks like it's getting consensus, or you can remove the functionality from this patch and land it sooner, then add it back in if `isa_or_null<>` gets adopted.
> Thanks again for the feedback -- especially on the matchers.
My pleasure!
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