[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Nathan James via Phabricator via cfe-commits]

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