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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
