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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Public-facing documentation should also be updated to specify the new option for the various checkers.


================
Comment at: clang-tidy/ClangTidy.h:55
@@ +54,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,
+  /// returns Default.
----------------
"in both options" -> "in either list"

================
Comment at: clang-tidy/ClangTidy.h:57
@@ +56,3 @@
+  /// returns Default.
+  std::string getLocalOrGlobal(StringRef LocalName, std::string Default) const;
+
----------------
Any reason for Default to not be a `StringRef` (I assume this is always going to be a hard-coded string literal in callers)? At the very least, it should be a `const std::string &`.

================
Comment at: clang-tidy/ClangTidyOptions.cpp:269
@@ +268,3 @@
+  SmallVector<llvm::StringRef, 5> Suffixes;
+  HeaderFileExtensions.split(Suffixes, ',');
+  for (const auto& Suffix : Suffixes) {
----------------
This code seems like it will fail if there are spaces in the extension list, like `.h, .hxx` instead of `.h,.hxx`. I think the list, as given by the end user, should be processed into a vector of sorts so that things can be normalized (entries without a period can be diagnosed, spaces removed, etc).

================
Comment at: clang-tidy/ClangTidyOptions.cpp:270
@@ +269,3 @@
+  HeaderFileExtensions.split(Suffixes, ',');
+  for (const auto& Suffix : Suffixes) {
+    if (Suffix.empty() && llvm::sys::path::extension(Filename).empty())
----------------
clang-format the patch.

================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+                                 llvm::StringRef HeaderFileExtensions);
----------------
endsWithHeaderFileExtension instead? However, given that uses of this all start with a SourceLocation, I wonder if that makes for a cleaner API: isLocInHeaderFile(SourceLocation, <Extensions>);

Also, how does this work if I want to include an extension-less file as the header file "extension?" It would be plausible if the extensions were passed in as a list, but as it stands it doesn't seem possible without weird conventions like leaving a blank in the list (e.g., `.h,,.hpp`), which seems error-prone.

Also, I'm not certain what I can pass in. The documentation should be updated to state whether these extensions are intended to include the ".".


Repository:
  rL LLVM

http://reviews.llvm.org/D16113