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