hintonda added a comment. In D59802#1447560 <https://reviews.llvm.org/D59802#1447560>, @aaron.ballman wrote:
> In D59802#1445566 <https://reviews.llvm.org/D59802#1445566>, @hintonda wrote: > > > I looked at the IR generated at `-O2`, and found that while `if > > (isa<X>(y))` is a modest win over `if (dyn_cast<X>(y)`, `if > > (dyn_cast_or_null<X>(y))` generates exactly the same IR that `if(y && > > isa<X>(y))` does. Also, if `y` is actually an expression that makes a > > function call, it's more expensive because it will make the call twice. > > > > So I don't seen any reason to replace `dyn_cast_or_null<>` in conditionals. > > > Mostly because I think it's more clear with `isa<>` because that's really > what it's checking. However, I could see not doing an automatic transform in > the case where the expression argument is something expensive, like a > function call. I could also see this as an impetus for adding `isa_or_null<>` > as a separate commit. My last patch only changes `if(y && isa<X>(y))` if `y` is a `CXXMemberCallExpr`, so I hope that addresses your concern. ================ 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: > 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. 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
