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

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 25 11:13:23 PDT 2015

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 


More information about the cfe-commits mailing list