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