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(),
----------------
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.
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
