[PATCH] D16113: [clang-tdiy] Add header file extension configuration support.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Sorry for the delay. A few more comments.


================
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.cpp:30
@@ +29,3 @@
+          RawStringHeaderFileExtensions, HeaderFileExtensions)) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
----------------
That's not the best way to report errors, especially, when clang-tidy is used as a library. There are multiple possibilities, e.g. add a callback to `ClangTidyContext` (or a method to `ClangTidyCheck`) to report a problem with configuration, or move to two-stage initialization of checks, or something similar. No need to change this now, but please add a FIXME to find a more suitable way to handle invalid configuration options.

================
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.cpp:59
@@ -45,1 +58,3 @@
+    if (!header_file_extensions_utils::isSpellingLocInHeaderFile(
+            D->getLocStart(), *(Result.SourceManager), HeaderFileExtensions))
       return;
----------------
No need for the parentheses around `Result.SourceManager`.

================
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:50
@@ +49,3 @@
+  HeaderFileExtensions.clear();
+  for (llvm::StringRef Suffix : Suffixes) {
+    llvm::StringRef Extension = Suffix.trim();
----------------
> seems that this is the first time to introduce list option into clang-tidy.

Comma is used as a delimiter in two more checks' options: clang-tidy/modernize/UseNullptrCheck.cpp and clang-tidy/misc/AssertSideEffectCheck.cpp. It's also used on the command line for the -checks and -warnings-as-errors options. I'm not sure what to do here, if we want consistency. For now, I'd rather switch back to comma and maybe provide a more generic facility to parse lists from clang-tidy options, which could also handle both separators in a smart way (e.g. when the list contains semicolons, use only them - this will allow listing entries like "A<B,T>", otherwise use comma; maybe also support some other separator, say `|` for cases when we need to list entries containing semicolons).

================
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.h:23
@@ +22,3 @@
+namespace tidy {
+namespace header_file_extensions_utils {
+
----------------
That's an unnecessary long and too specific namespace name. I'd go for just `utils`, at least for now.

================
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.h:33
@@ +32,3 @@
+/// \brief Checks whether presumed location of Loc is in header file.
+bool isPresumedLocInHeaderFile(
+    SourceLocation Loc, SourceManager &SM,
----------------
Don't add unused functions. When someone needs them, they can add them.


http://reviews.llvm.org/D16113