alexfh added a comment.
Looks good with a few nits. Please fix them and I'll submit the patch for you.
Thank you for working on this!
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:23
@@ +22,3 @@
+AST_MATCHER(NamedDecl, isHeaderFileExtension) {
+ SourceManager& SM = Finder->getASTContext().getSourceManager();
+ SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
----------------
Actually, this name is also confusing, but I don't have a better idea now.
Let's leave it like this.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:25
@@ +24,3 @@
+ SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
+ if (SM.isInSystemHeader(ExpansionLoc))
+ return false;
----------------
I think, this should be opposite for two reasons:
1. variable definitions and declarations in system headers are not better than
in user headers and can lead to the same problems;
2. there are people who care about system headers, so we need to produce
diagnostics on them and let clang-tidy filter them out when not needed.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.h:23
@@ +22,3 @@
+// There is one option:
+// - `UseHeaderFileExtension`: Whether to use file extension(h, hh, hpp, hxx)
to
+// distinguish header files. True by default.
----------------
nit: add a space before the '('
================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:15
@@ +14,3 @@
+
+ // Internal linkage variable definitions are ignored for now,
+ // Although these might also cause ODR violations, we can be less certain
and
----------------
nit: the sentence should end with a period, not a comma.
================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:4
@@ +3,3 @@
+int f() {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header
file;
+// CHECK-FIXES: inline int f() {
----------------
Please specify each distinct warning message (in your case, there are two of
them) completely (including the check name) once. Having it in the test (just
once) is useful to verify that the message is formatted correctly (in your
case, there's no space after the ';', for example).
http://reviews.llvm.org/D15710
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
