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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for the patch! I have a few concerns though. See inline comments.


================
Comment at: clang-tidy/ClangTidy.cpp:348
@@ +347,3 @@
+                                     LocalName.str() };
+  for (const auto &Name : Names) {
+    auto Iter = CheckOptions.find(Name);
----------------
There's no need to declare a vector: `for (const string &s : {"a", "b", "c"}) ...`.

Also, I don't think it makes sense to create a vector of two elements and run a loop over it instead of duplicating three lines of code.

================
Comment at: clang-tidy/ClangTidy.h:54
@@ +53,3 @@
+  ///
+  /// Reads the option with the check-local name \p LocalName from local or
+  /// global \c CheckOptions. If the option is not present in both options,
----------------
We need to explicitly specify the order in which the options are considered: local, global, default.

================
Comment at: clang-tidy/ClangTidyOptions.cpp:269
@@ +268,3 @@
+  SmallVector<llvm::StringRef, 5> Suffixes;
+  HeaderFileExtensions.split(Suffixes, ',');
+  for (const auto& Suffix : Suffixes) {
----------------
It's rather inefficient to split the string each time the function is called. If even we needed this function, we would better pass an `ArrayRef<StringRef>`. But even better, we can use `llvm::StringSet` and get rid of this function at all (we might need a helper function to parse a comma-separated list of extensions to a StringSet though).

================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+                                 llvm::StringRef HeaderFileExtensions);
----------------
This function doesn't belong here. I'm also not sure we need this function at all. First, it's ineffective to split the string containing the list of extensions each time. Second, if we store a set of extensions, then we can just search for the actual file extension in this set.

================
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.h:23
@@ +22,3 @@
+///
+/// There is one option:
+/// - `HeaderFileExtensions`: the file extensions that regard as header file.
----------------
nit: s/There is one option:/The check supports these options:/

================
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.h:24
@@ +23,3 @@
+/// There is one option:
+/// - `HeaderFileExtensions`: the file extensions that regard as header file.
+///   ".h" by default.
----------------
s/the file extensions that regard as header file/a comma-separated list of filename extensions of header files/

I don't think this list should include the period. It's better to just store extensions as returned by `llvm::sys::path::extension()`, so that it can be simply looked up in a set.

Also, it's common to indent lists, so please insert two spaces at the beginning of this line and the next one.

================
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.h:33
@@ -28,1 +32,3 @@
+private:
+  const std::string HeaderFileExtensions;
 };
----------------
As mentioned earlier, this can be a set of strings (e.g. `llvm::StringSet`).

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:22
@@ -21,2 +21,3 @@
 
-AST_MATCHER(NamedDecl, isHeaderFileExtension) {
+AST_MATCHER_P(NamedDecl, useHeaderFileExtension,
+              StringRef, HeaderFileExtensions) {
----------------
s/useHeaderFileExtension/usesHeaderFileExtension/

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:78
@@ +77,3 @@
+static cl::opt<std::string>
+HeaderFileExtensions("header-file-extensions",
+                     cl::desc("File extensions that regard as header file.\n"
----------------
We don't need a command-line flag for this. The regular way to configure clang-tidy is .clang-tidy files. However, if needed, check options can be configured from the command line via the `-config=` option. 


Repository:
  rL LLVM

http://reviews.llvm.org/D16113