alexfh added a comment.
A couple of post-commit comments.
Comment at:
clang-tools-extra/trunk/clang-tidy/google/GlobalNamesInHeadersCheck.h:24
@@ +23,3 @@
+///
+/// The check supports these options:
+/// - `HeaderFileExtensions`: a comma-separated list of filename extensions
This revision was automatically updated to reflect the committed changes.
Closed by commit rL259879: [clang-tdiy] Add header file extension configuration
support. (authored by hokein).
Changed prior to commit:
http://reviews.llvm.org/D16113?vs=46923&id=47007#toc
Repository:
rL LLVM
http://r
alexfh added a comment.
Still looks good.
http://reviews.llvm.org/D16113
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hokein marked 2 inline comments as done.
hokein added a comment.
http://reviews.llvm.org/D16113
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hokein updated this revision to Diff 46923.
hokein added a comment.
Update.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
clang-tidy/google/UnnamedName
alexfh added inline comments.
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:46
@@ +45,3 @@
+bool parseHeaderFileExtensions(llvm::StringRef AllHeaderFileExtensions,
+ HeaderFileExtensionsSet &HeaderFileExtensions,
+
hokein added inline comments.
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:46
@@ +45,3 @@
+bool parseHeaderFileExtensions(llvm::StringRef AllHeaderFileExtensions,
+ HeaderFileExtensionsSet &HeaderFileExtensions,
+
alexfh added inline comments.
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:40
@@ +39,3 @@
+',')) {
+// FIXME(hokein): Find a more suitable way to handle invalid configuration
+// options.
It's common to
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
Looks good with a couple of comments. Thanks for working on this!
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:45
@@ +44,3 @@
+
+bool parseHeaderFileExtensions(l
hokein marked 3 inline comments as done.
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:51
@@ +50,3 @@
+ HeaderFileExtensions.clear();
+ for (llvm::StringRef Suffix : Suffixes) {
+llvm::StringRef Extension = Suffix.trim();
Thanks for the explanat
hokein updated this revision to Diff 46897.
hokein added a comment.
Address Alex's comments.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
clang-tidy/g
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: "
+
hokein added a comment.
ping @alexfh.
http://reviews.llvm.org/D16113
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you for working on this!
http://reviews.llvm.org/D16113
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://
hokein updated this revision to Diff 45852.
hokein added a comment.
Change delimeter from "," to ";".
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
cla
hokein marked an inline comment as done.
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:49
@@ +48,3 @@
+ AllHeaderFileExtensions.split(Suffixes, ',');
+ HeaderFileExtensions.clear();
+ for (llvm::StringRef Suffix : Suffixes) {
I'm +1 on keeping the
aaron.ballman added a comment.
Mostly looks good, except I do still have the question about list delimiters
that I'd like to hear some thoughts on. My primary concern is that I hope to
avoid having different separators for different lists, and I wonder whether we
want a common "parse a list of
hokein marked 3 inline comments as done.
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.h:26
@@ +25,3 @@
+/// - `HeaderFileExtensions`: a comma-separated list of filename extensions
of
+/// header files (no need to includ "."). "h" by default.
+/// For extensio
hokein updated this revision to Diff 45349.
hokein added a comment.
Update comments.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
clang-tidy/google/Un
aaron.ballman added inline comments.
Comment at: clang-tidy/google/GlobalNamesInHeadersCheck.h:26
@@ +25,3 @@
+/// - `HeaderFileExtensions`: a comma-separated list of filename extensions
of
+/// header files (no need to includ "."). "h" by default.
+/// For extension-le
hokein updated this revision to Diff 45077.
hokein marked an inline comment as done.
hokein added a comment.
Format code style.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalN
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/HeaderFileExtensionsUtils.cpp:48
@@ +47,3 @@
+
+bool parseHeaderFileExtensions(llvm::StringRef AllHeaderFileExtensions,
+ HeaderFileExtensionsSet &HeaderFileExtensions) {
--
hokein marked 2 inline comments as done.
Comment at: clang-tidy/ClangTidyOptions.cpp:269
@@ +268,3 @@
+ SmallVector Suffixes;
+ HeaderFileExtensions.split(Suffixes, ',');
+ for (const auto& Suffix : Suffixes) {
aaron.ballman wrote:
> alexfh wrote:
> > It's rath
hokein updated this revision to Diff 45006.
hokein marked an inline comment as done.
hokein added a comment.
Add extension-less header file doc.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-ti
hokein marked 11 inline comments as done.
Comment at: clang-tidy/ClangTidy.h:65
@@ -56,3 +64,3 @@
/// \c CheckOptions. If the corresponding key is not present, returns
/// \p Default.
template
Makes sense. I just refer from `std::string get(StringRef Loc
hokein updated this revision to Diff 44951.
hokein added a comment.
Address review comments.
http://reviews.llvm.org/D16113
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
clang-tidy/g
alexfh added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
aaron.ballman w
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
alexfh w
alexfh added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
aaron.ballman w
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
alexfh w
alexfh added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:78
@@ +77,3 @@
+static cl::opt
+HeaderFileExtensions("header-file-extensions",
+ cl::desc("File extensions that regard as header file.\n"
hokein wrote:
> alexfh wrote:
hokein added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:78
@@ +77,3 @@
+static cl::opt
+HeaderFileExtensions("header-file-extensions",
+ cl::desc("File extensions that regard as header file.\n"
alexfh wrote:
> We don't need
alexfh added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
hokein wrote:
>
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.fi
hokein added inline comments.
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
aaron.ballman w
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
36 matches
Mail list logo
