[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/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") ||
----------------
aaron.ballman wrote:
> alexfh wrote:
> > 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).
> I would argue that if you run clang-tidy on just the header file, you might know what you are doing and expect to not see those diagnostics at all.
> 
> Relying on the extension makes this far less useful, IMO. For instance, even on this second try, the checker will fail to catch ODR violations in STL-style header files. You can #include "file.cpp" just as easily as "file.h" (as we do with .inc files, for instance), and that can still result in ODR violations too. To me, the extension isn't what makes this checker do interesting work, it's the fact that these can only be violations when the file is included in multiple TUs (except we can only check one TU at a time, so looking for the file to be included is the best we can do). 
> I would argue that if you run clang-tidy on just the header file, you might know what you are doing and expect to not see those diagnostics at all.

I don't agree here. What one runs the check on is a purely technical detail. It may be even hidden from the user by tooling (e.g. editor or code review integration).

> Relying on the extension makes this far less useful, IMO. For instance, even 
> on this second try, the checker will fail to catch ODR violations in STL-style 
> header files.

We can make it work for STL-style header files by treating extensionless files as headers. Or make this filter a regular expression or something like that.

> You can #include "file.cpp" just as easily as "file.h" (as we do with .inc files, for instance), and that can still result in ODR violations too.

We don't usually include `.cpp` files. And the `.inc` extension says the file is a textual include and usually can't be parsed on its own. And it's very uncommon to actually compile `.h` files on their own as a part of the build. So I'm inclined to treat the file extension as a valuable piece of information regarding its intended use.

> To me, the extension isn't what makes this checker do interesting work, it's the 
> fact that these can only be violations when the file is included in multiple TUs 

If someone includes a ".cpp" file, it's a problem, but, IMO, detection of actually present ODR violations is a task for a different tool (possibly, another clang-tidy check, once we have multi-translation-unit support). This check's purpose is to detect a certain class of issues that can potentially cause ODR violations.

> (except we can only check one TU at a time, so looking for the file to be 
> included is the best we can do).

We can do even better by utilizing the information we already have in the file name. Do you see any specific issues that can result from this?

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:31
@@ +30,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      decl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())))
----------------
aaron.ballman wrote:
> alexfh wrote:
> > Do you expect this to improve performance?
> Possibly improve performance (I would suspect there are a lot of varDecls and a fair number of functionDecls in the main file), but I also prefer the checkers to be as declarative as possible. It makes it easier (at least for me) to reason about what AST nodes the check is ultimately interested in.
As noted above, "main file" is not a useful condition here. However, it's certainly possible to move some of the checks from the `check` method to matcher(s) (e.g. `isLocationInHeaderFile`). I don't feel strongly about that.


http://reviews.llvm.org/D15710