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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
