This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix relative path in header-filter.
ClosedPublic

Authored by xyb on Sep 12 2019, 7:52 AM.

Details

Summary

Clang-tidy supports output diagnostics from header files if user
specifies --header-filter. But it can't handle relative path well.
For example, the folder structure of a project is:

// a.h is in /src/a/a.h
#include "../b/b.h"

// b.h is in /src/b/b.h
...

// c.cpp is in /src/c.cpp
#include "a/a.h"

Now, we set --header-filter as --header-filter=/a/. That means we only
want to check header files under /src/a/ path, and ignore header files
uder /src/b/ path, but in current implementation, clang-tidy will check
/src/b/b.h also, because the name of b.h used in clang-tidy is
/src/a/../b/b.h.

This change tries to fix this issue.

Diff Detail

Repository
rL LLVM

Event Timeline

xyb created this revision.Sep 12 2019, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 7:52 AM
Eugene.Zelenko added a project: Restricted Project.
gribozavr added inline comments.Sep 20 2019, 1:58 AM
clang-tools-extra/test/clang-tidy/file-filter.cpp
60 ↗(On Diff #219919)

Could you rename header.h files to header_a.h, header_b.h etc. to make it obvious in CHECK lines where the messages are coming from?

MyDeveloperDay added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
555 ↗(On Diff #219919)

elide the {}

xyb updated this revision to Diff 220997.Sep 20 2019, 4:18 AM
xyb marked 2 inline comments as done.

Updated

gribozavr accepted this revision.Sep 20 2019, 5:46 AM

Thanks! Please let me know if you need me to commit the patch for you.

This revision is now accepted and ready to land.Sep 20 2019, 5:46 AM
xyb added a comment.Sep 20 2019, 5:52 AM

Yes, I need your help to submit the patch. I don't have the permission. Thanks.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 6:20 AM

Sorry, I reverted this patch in r372601.

Unfortunately, it makes paths printed in clang-tidy'd diagnostics inconsistent with what -header-filter operates on.

For example, imagine that file-filter.cpp includes header_alias.h, which is a symlink to header.h. The diagnostics printed by clang-tidy refer to header_alias.h, however, after this patch, -header-filter logic calls realpath() and operates on header.h -- making it very difficult for users to understand how exactly to set up filters.

Also, note that paths in diagnostics don't collapse foo/.. or symlinks.

We should only change both sides of path handling simultaneously (paths in diagnostics and paths in filters). However, since getting the user's preferred path is potentially very difficult when symlinks are present, I'm not sure if anything can be done here. Maybe only collapsing foo/.. would be viable?

I added a test in r372607 for your reference. It tests both foo/.. and symlink behavior.

Maybe only collapsing foo/.. would be viable?

@ilya-biryukov told me that even that is not viable, in case foo is a symlink to a directory.

The only workable suggestion that we could come up with was adding a separate command-line option, -normalized-header-filter, that would normalize paths before matching the regex. It can be used by people whose project layout does not have complex symlink mazes.

Sorry, I reverted this patch in r372601.

Unfortunately, it makes paths printed in clang-tidy'd diagnostics inconsistent with what -header-filter operates on.

For example, imagine that file-filter.cpp includes header_alias.h, which is a symlink to header.h. The diagnostics printed by clang-tidy refer to header_alias.h, however, after this patch, -header-filter logic calls realpath() and operates on header.h -- making it very difficult for users to understand how exactly to set up filters.

Also, note that paths in diagnostics don't collapse foo/.. or symlinks.

We should only change both sides of path handling simultaneously (paths in diagnostics and paths in filters). However, since getting the user's preferred path is potentially very difficult when symlinks are present, I'm not sure if anything can be done here. Maybe only collapsing foo/.. would be viable?

Even collapsing foo/.. may be problematic , if foo is a symlink.

$ mkdir -p a/b/c
$ ln -s a/b/c d
$ ls
a d
$ ls a/..
a d
$ ls d/..
c
xyb added a comment.Sep 23 2019, 7:03 AM

The only workable suggestion that we could come up with was adding a separate command-line option, -normalized-header-filter, that would normalize paths before matching the regex. It can be used by people whose project layout does not have complex symlink mazes.

-normalized-header-filter. I'd like this idea. Any objections?

In D67501#1679050, @xyb wrote:

-normalized-header-filter. I'd like this idea. Any objections?

Having two solution for filter is unfortunate - making both discoverable and documenting why we need them is hard.
If there are important use-cases that can't be served otherwise, I guess there's no way to avoid it.

However, if there are workaround (even if they're slightly annoying), it's much simpler to have a single option.