njames93 requested changes to this revision.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:45
+
+inline SourceRange getAnglesLoc(const Expr *MatchedCallee) {
+ if (const auto *DeclRefExprCallee =
----------------
Use `static` instead of `inline`. Inline should only be used for functions(or
variables) defined in header files.
Same goes for the other `inline` functions.
Also this function should be moved outside the anonymous namespace.
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:47
+ if (const auto *DeclRefExprCallee =
+ llvm::dyn_cast_or_null<DeclRefExpr>(MatchedCallee))
+ return SourceRange(DeclRefExprCallee->getLAngleLoc(),
----------------
Remove the `llvm::` qualifier and the null check is redundant here as its
guaranteed to never be called with a non-null argument.
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:52
+ if (const auto *UnresolvedLookupExprCallee =
+ llvm::dyn_cast_or_null<UnresolvedLookupExpr>(MatchedCallee))
+ return SourceRange(UnresolvedLookupExprCallee->getLAngleLoc(),
----------------
Ditton
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:94-96
+ Finder->addMatcher(
+ traverse(
+ TK_IgnoreUnlessSpelledInSource,
----------------
Rather than calling traverse, the canoncial way to handle this is by overriding
the `getCheckTraversalKind` virtual function to return
`TK_IgnoreUnlessSpelledInSource`
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:99
+ unless(isExpansionInSystemHeader()), argumentCountIs(1U),
+ IgnoreDependentExpresions
+ ? expr(unless(isInstantiationDependent()))
----------------
ccotter wrote:
> I think we might need more tests when `IgnoreDependentExpresions` is true.
> When I was playing around and hardcoded IgnoreDependentExpresions to false on
> line 99, the tests still pass.
This needs addressing before this can be landed
================
Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:107
+ expr(anyOf(declRefExpr(hasDeclaration(functionDecl(
+ hasName("::std::forward"))),
+ hasTemplateArgumentLoc(
----------------
It's a good idea to follow to DRY principle:
```lang=c++
auto IsNamedStdForward = hasName("::std::forward");
```
Then you can use that here and below.
A similar point can be made with the bind IDs, if you define the IDs as a
`static constexpr char []` you can reuse them during`bind` and `getNodeAs`
calls, helps avoid bugs if the check is ever upgraded.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144347/new/
https://reviews.llvm.org/D144347
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
