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

[Aaron Ballman via cfe-commits]

aaron.ballman 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") ||
----------------
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). 

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:31
@@ +30,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      decl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())))
----------------
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.


http://reviews.llvm.org/D15710