[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:
> 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?
>> 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.

The point I was getting at is that an extension is arbitrary, though somewhat conventionally-named. I don't see it as being particularly valuable in this case because "has this file been included" is sufficient information to determine whether we want to diagnose or not. I guess that these arguments all boil down to which is more likely to happen: a header file with an extension we don't capture (because user's aren't going to *know* to customize the header file extension list, generally), or the user running this checker on just the header file, but never a source file that includes the header file. While I can think of cases where the user runs it on just the header file, they seem few and far between.

Perhaps another solution to this is use isInMainFile() || usesHeaderFileExtension().

> 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?

The file extension is never going to be sufficient, or even accurate, information because the extensions are just conventions and not everyone follows the typical ones. For instance, you said that .inc files usually cannot stand on their own, but that isn't strictly followed even in our project (some of the table-generated source files are .inc files that stand alone). Since this check's predominate purpose is to prevent possible ODR violations, inclusion gives more pertinent information, IMO.


http://reviews.llvm.org/D15710