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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
