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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


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


================
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:
> > 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.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:103-104
+
+    //    if (StartLoc.isMacroID())
+    //  return;
+
----------------
Spurious code that can be removed?


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:118
+         "cast<> in conditional will assert rather than return a null pointer")
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =
----------------
Aside from the fix-it, this code is identical to the block above. Can be combined.


================
Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:125-126
+
+    diag(MatchedDecl->getBeginLoc(), "return value from dyn_cast<> not used")
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =
----------------
There are zero test cases that seem to trigger this diagnostic text.


================
Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet<StringRef, 5>;
 
----------------
Can you add back the `::llvm::` qualifier on the `StringRef` type?


================
Comment at: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:22
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast<X>(y))
----------------
Please spell out the full diagnostic the first time it occur in the test case - it's fine to abbreviate subsequent diagnostics, but we like to see at least one exact match per diagnostic.


================
Comment at: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:48
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa<X>(y))
----------------
Same here.


================
Comment at: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:64
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null<Y>(z->bar()))
----------------
and here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802