[PATCH] D15710: [clang-tidy] Add non-inline function definition and variable definition check in header files.

[Alexander Kornienko via cfe-commits]

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