alexfh added inline comments.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:52
@@ +51,3 @@
+ // Inline function is allowed.
+ if (funDecl->isInlined())
+ return;
----------------
hokein wrote:
> alexfh wrote:
> > This check can be done in the matcher.
> The `isInline` AST matcher seems only match the function with `inline` key
> words. It could not detect the member function defined within class, such as:
>
> ```
> class A {
> int f() { return 1; }
> };
> ```
Yep, I've found this out myself, but have forgotten to remove the comment.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:23
@@ +22,3 @@
+bool inHeaderFile(const SourceManager *SM, SourceLocation Location) {
+ StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location));
+ return Filename.endswith(".h") || Filename.endswith(".hh") ||
----------------
I don't think "main file" is the right condition here. If we run clang-tidy on
a header file, it will be the main file, but we still are interested in the
diagnostics in it. So I don't see a better way to distinguish header files than
looking at their extension (some projects might need the list of extensions to
be configurable, but that's another story).
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:31
@@ +30,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ decl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())))
----------------
Do you expect this to improve performance?
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:47
@@ +46,3 @@
+ const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("decl");
+ if (!ND)
+ return;
----------------
I'd replace this with an assertion, since this bound node should always be a
`NamedDecl`. To make this explicit, I'd also use the `namedDecl` matcher
instead of `decl` above.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:53
@@ +52,3 @@
+ // const int a = 1;
+ // static int a = 1;
+ // namespace {
----------------
nit: Use of the same variable name distracts from the essence of the example.
Please use unique variable names.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:56
@@ +55,3 @@
+ // int a = 1;
+ // int f() { return 1}
+ // }
----------------
nit: Missing `; ` after `1`.
================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:2
@@ +1,3 @@
+misc-definitions-in-headers
+=============================
+
----------------
nit: Please remove two `=`'s.
================
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:140
@@ +139,3 @@
+ "}", "foo.h"));
+ EXPECT_EQ(errorMessage, runCheckOnCode("template <typename T>\n"
+ "T f() {\n"
----------------
hokein wrote:
> alexfh wrote:
> > I think, we can already use raw string literals (at least, MSVC 2013 seems
> > to support them). Raw string literals would make the test cases more
> > readable, imo.
> The Raw string literals doesn't work well with multiple lines in macro. I try
> it on my local machine, it will break compilation.
>
You mean, the `EXPECT_EQ` macro doesn't work with raw string literals somehow?
That sounds strange.
================
Comment at: unittests/clang-tidy/MiscModuleTest.cpp:38
@@ -36,1 +37,3 @@
+class DefinitionsInHeadersCheckTest : public ::testing::Test {
+protected:
----------------
aaron.ballman wrote:
> hokein wrote:
> > I'm not sure about this. The reason I put the test in unittest is that I
> > see the other header checks(`GlobalNamesInHeadersCheck`) are also in the
> > unittest. It would be better to keep the consistency.
> I was arguing for consistency as well -- clang-tidy tests are run via lit
> instead of unit tests unless there's a reason to do otherwise. It sounds like
> there's no reason for these to be unit tests.
I don't feel strongly about this, but I generally prefer lit tests as well,
since they allow to test both messages and fixes easily and with less
boilerplate. It's true, that setting up a test case with multiple header files
may be somewhat easier in a unit test (where everything is described in a
single place), but this doesn't seem to be the case.
Once you start testing fixits, a lit test will immediately become a simpler
solution.
http://reviews.llvm.org/D15710
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
