[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:73
+          Result.Nodes.getNodeAs<CallExpr>("cast-assign")) {
+    auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+    auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
----------------
hintonda wrote:
> Eugene.Zelenko wrote:
> > Please don't use auto where type is not obvious.
> Happy to make the change, but thought the `get.*Loc()` methods were obvious?
Generally, no. There are a fair number of different location types and it's not obvious which type you're getting. We usually mean obvious as in "this type is an iterator, but the concrete type of the iterator isn't that important" or "the type is exactly spelled out as a template argument".


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:38
+      whileStmt(anyOf(
+          has(declStmt(containsDeclaration(
+              0, varDecl(
----------------
This seems like a fair amount of code duplication -- can this be cleaned up using some local variables for the inner matchers?


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:60
+      binaryOperator(
+          allOf(hasOperatorName("&&"), hasLHS(expr().bind("lhs")),
+                hasRHS(anyOf(
----------------
I think the `expr()` matcher is too general. This will trigger on something like `if (foo && isa<T>(bar))` won't it?

I'd also like to see tests for code like:
```
foo && cast<T>(foo)->bar();
```
I don't think this pattern should diagnose because you cannot safely replace it with `dyn_cast<T>(foo)->bar()`


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:62-64
+                    callExpr(
+                        allOf(hasArgument(0, expr().bind("arg")),
+                              callee(namedDecl(hasName("isa")).bind("func"))))
----------------
I do not think we should diagnose code that uses `obj && isa<T>(obj)` -- that's very reasonable code.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:90-91
+         "cast<> in conditional will assert rather than return a null pointer")
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc),
+                                        "dyn_cast");
+  } else if (const auto *MatchedDecl =
----------------
Should we be concerned about fix-it interactions with macros (here and elsewhere in the check)? We usually suppress fix-its if the replacement range is somewhere within a macro expansion.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:96
+    SourceLocation EndLoc =
+        StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+
----------------
What if the reason we matched was because the `cast` was written using a macro of a different name? e.g., `#define LONGER_IDENTIFIER(T, Obj) cast<T>(Obj)`


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:112-116
+    // Don't touch Casting.h, since it would math otherwise.
+    const StringRef Casting("llvm/include/llvm/Support/Casting.h");
+    if (Result.SourceManager->getFilename(MatchedDecl->getBeginLoc())
+            .take_back(Casting.size()) == Casting)
+      return;
----------------
Can this be handled at the matcher level? IIRC we have `isExpansionInFileMatching()` or something like that.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+    diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
----------------
This diagnostic doesn't tell the user what they've done wrong with the code or why this is a better choice.


================
Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:256-258
+  if args.fix:
+    check_clang_apply_replacements_binary(args)
+
----------------
Is this related to your patch? If so, why is it needed?


================
Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet<StringRef, 5>;
 
----------------
Personally, I prefer using `::llvm::SmallSet` to this change. It makes it very clear what namespace the type lives in.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802