[PATCH] D19259: Initial version of misc-unused-using-decl check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Awesome! Thank you for tackling this! A few comments.


================
Comment at: clang-tidy/misc/CMakeLists.txt:38
@@ -37,2 +37,3 @@
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
+  UnusedUsingDeclsCheck.cpp
----------------
Please fix file sorting around the new file.

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:123
@@ -121,1 +122,3 @@
+    CheckFactories.registerCheck<UnusedUsingDeclsCheck>(
+        "misc-unused-using-decls");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
----------------
I think, all "misc-unused-" checks except for the "misc-unused-raii" (which finds actual bugs) mostly relate to readability. I suggest placing the new check to readability/ and we'll then move the other two "misc-unused-" checks there as well.

================
Comment at: clang-tidy/misc/UnusedUsingDeclsCheck.cpp:23
@@ +22,3 @@
+  Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
+  Finder->addMatcher(recordType(hasDeclaration(namedDecl().bind("used"))),
+                     this);
----------------
Why is it necessarily a `recordType`? How about typedefs and enums?

================
Comment at: clang-tidy/misc/UnusedUsingDeclsCheck.h:32
@@ +31,3 @@
+private:
+  llvm::DenseMap<const NamedDecl*, const UsingDecl*> FoundDecls;
+  llvm::DenseMap<const NamedDecl*, CharSourceRange> FoundRanges;
----------------
I'm not sure how the llvm/ADT/DenseMap.h header is pulled here, but DenseMap is not even used in ClangTidy.h, so this transitive inclusion is rather brittle. Please include llvm/ADT/DenseMap.h directly.

================
Comment at: docs/clang-tidy/checks/misc-unused-using-decls.rst:6
@@ +5,2 @@
+
+Finds unused using declarations.
----------------
Add an example, please.


http://reviews.llvm.org/D19259