This is an archive of the discontinued LLVM Phabricator instance.

[clang][HeaderSearch] Shorten paths for includes in mainfile's directory
ClosedPublic

Authored by kadircet on Jun 13 2019, 12:38 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Jun 13 2019, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 12:38 PM
gribozavr accepted this revision.Jul 1 2019, 5:44 AM

I'm not an expert in this code, but it looks reasonable.

clang-tools-extra/clangd/Headers.h
155 ↗(On Diff #204608)

Please don't write what something is used for, write what it is. You can explain how it affects the output of the function of course, but don't just say "used for" -- like what you did in clang/include/clang/Lex/HeaderSearch.h.

clang-tools-extra/clangd/unittests/HeadersTests.cpp
215 ↗(On Diff #204608)

This test name should probably mention the fact that the directory is on the include path.

225 ↗(On Diff #204608)

The test name probably should be adjusted.

clang/lib/Lex/HeaderSearch.cpp
1683 ↗(On Diff #204608)

I'd suggest to spell the return type, and document what it means.

This revision is now accepted and ready to land.Jul 1 2019, 5:44 AM
hokein added a comment.Jul 1 2019, 7:40 AM

Discussed it offline, technically taking the main file directory into account when shorten the #include path would not break the compiler, but it may introduce issues violating the code style -- in clangd codebase, we prefer to use "#include "<subdir_under_clangd>/header.h" style, e.g. index/background.cc uses #include "index/Background.h", with this patch we will shorten the include as #include "background.h", so we don't count the the index/background.h as included (a new #include "background.h" will be inserted).

One possible solution is that we could use the main file directory as a last fallback (if we are not able to shorten the #included path using any search-header directories, we try to use the main-file directory).

kadircet updated this revision to Diff 207308.Jul 1 2019, 7:52 AM
kadircet marked 3 inline comments as done.
  • Address comments.
  • Use TUs path as a fallback, rather than a search path.
kadircet updated this revision to Diff 207312.Jul 1 2019, 7:57 AM
  • Add comments to the CheckDir lambda.
hokein added a comment.Jul 2 2019, 2:28 AM

looks mostly good, a few nits.

clang/include/clang/Lex/HeaderSearch.h
724 ↗(On Diff #207312)

I think we should have some documentation about this new behavior in the comment here.

clang/lib/Sema/SemaLookup.cpp
5245 ↗(On Diff #207312)

it seems not safe, getFileEntryForID may return a nullptr...

clang/unittests/Lex/HeaderSearchTest.cpp
124 ↗(On Diff #207312)

nit: add /*MainFile=*/ comment annotation to improve the code readability.

kadircet updated this revision to Diff 207497.Jul 2 2019, 3:00 AM
kadircet marked 4 inline comments as done.
  • Address comments
hokein accepted this revision.Jul 3 2019, 12:33 AM
hokein added inline comments.
clang-tools-extra/clangd/Headers.h
155 ↗(On Diff #207497)

nit: instered => inserted.

This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 12:47 AM