[PATCH] D13081: [clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Awesome! This makes the check far more usable.

See a few minor comments in-line.


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:140
@@ +139,3 @@
+  Finder->addMatcher(
+      namedDecl(unless(isExpansionInSystemHeader())).bind("decl"), this);
+  Finder->addMatcher(
----------------
ClangTidy takes care of filtering errors. It doesn't show errors in system headers by default, but there's an option to show them, if needed (e.g. for library developers). Is there any reason to add filtering here as well?

================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:537
@@ +536,3 @@
+
+    return addUsage(NamingCheckFailures, Decl->getParent(),
+                    Decl->getNameInfo().getSourceRange(), Result.SourceManager);
----------------
Please split `return` and `addUsage` here and in other places. It looks rather confusing to me and adds no benefits.

================
Comment at: test/clang-tidy/readability-identifier-naming.cpp:69
@@ -72,1 +68,3 @@
 
+#include <iostream>
+// Expect NO warnings or errors from system headers, it shouldn't even be checked
----------------
Please don't #include system headers to tests. There are multiple reasons to avoid this:
  * that won't work in some test configurations;
  * we don't control the contents of system headers, so we can't make tests reliable;
  * testing time can increase significantly due to the possibly large size of system headers.

If you need to test includes, put mock headers to test/clang-tidy/Inputs/<check-name>/ and specify this directory in the clang-tidy invocation:

  // RUN: %python %S/check_clang_tidy.py %s readability-identifier-naming %t \
  // RUN:   -config=... \
  // RUN:   -- -std=...  -isystem=%S/Inputs/readability-identifier-naming 


http://reviews.llvm.org/D13081