[PATCH] D97361: [clang-tidy] Add misc-redundant-using check

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:23
+  Finder->addMatcher(
+      usingDecl(isExpansionInMainFile()).bind("using-declaration"), this);
+  Finder->addMatcher(
----------------
Why restrict these to main file only?


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:30-33
+  if (const auto *Declaration =
+          Result.Nodes.getNodeAs<UsingDecl>("using-declaration")) {
+    checkUsingDecl(Declaration, Result);
+  }
----------------
nit: Elide braces of single line if statements.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:35-38
+  if (const auto *Directive =
+          Result.Nodes.getNodeAs<UsingDirectiveDecl>("using-directive")) {
+    checkUsingDirective(Directive, Result);
+  }
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:45
+
+  for (const auto *UsingShadow : Declaration->shadows()) {
+    const Decl *TargetDecl = UsingShadow->getTargetDecl()->getCanonicalDecl();
----------------
nit: don't use auto as type isn't explicitly spelled in the initialization.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:50
+      const auto *NSD = cast<NamespaceDecl>(TargetDecl->getDeclContext());
+      diagUsing("declaration", Declaration, Declaration, NSD, Result);
+    }
----------------
Shouldn't there be a break after we have diagnosed this?


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp:78
+  diag(Using->getLocation(),
+       "using %0 %1 is redundant, already in namespace %2")
+      << Type << ND << NSD;
----------------
nit: The canonical way to do diagnostics like this is with select. Then pass 0 for declaration and 1 for directive.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97361