LegalizeAdulthood added inline comments.
================
Comment at: clang-tidy/readability/DuplicateIncludeCheck.cpp:62
@@ +61,3 @@
+ StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+ if (!SM_.isInMainFile(HashLoc)) {
+ return;
----------------
LegalizeAdulthood wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > LegalizeAdulthood wrote:
> > > > alexfh wrote:
> > > > > What's the reason to limit the check to the main file only? I think,
> > > > > it should work on all headers as well. Also, sometimes it's fine to
> > > > > have duplicate includes even without macro definitions in between,
> > > > > e.g. when these #includes are in different namespaces.
> > > > >
> > > > > I'd suggest using the same technique as in the IncludeOrderCheck: for
> > > > > each file collect all preprocessor directives sorted by
> > > > > SourceLocation. Then detect #include blocks (not necessarily the same
> > > > > way as its done in the IncludeOrderCheck. Maybe use the presense of
> > > > > any non-comment tokens between #includes as a boundary of blocks),
> > > > > and detect duplicate includes in each block.
> > > > If I remove the `isInMainFile`, how do I ensure that I don't attempt to
> > > > modify system headers?
> > > Using `SourceLocation::isInSystemHeader()`.
> > Thanks, I'll try that. The next question that brings to mind is how do I
> > distinguish between headers that I own and headers used from third party
> > libraries?
> >
> > For instance, suppose I run a check on a clang refactoring tool and it uses
> > `isInSystemHeader` and starts flagging issues in the clang tooling library
> > headers.
> >
> > The `compile_commands.json` doesn't contain any information about headers
> > in my project, only translation units in my build, so it doesn't know
> > whether or not included headers belong to me or third-party libraries.
> For the benefit of others reading this, Alex pointed out to me the
> `-header-filter` and `-system-headers` options to clang-tidy. I think this
> means I don't need any narrowing if `isExpansinInMainFile()` in any of my
> matchers. I will do some experimenting to verify that this doesn't introduce
> regressions.
If I remove the `isInMainFile` and replace it with a check to
`isInSystemHeader`, then all my tests fail. I will have to investigate this
further.
http://reviews.llvm.org/D7982
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
