This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Allow using customized include path in URI.
ClosedPublic

Authored by hokein on Apr 9 2018, 2:23 AM.

Details

Summary

Calculating the include path from absolute file path does not always
work for all build system, e.g. bazel uses symlink as the build working
directory. The absolute file path from editor and clang is diverged from
each other. We need to address it properly in build sysmtem integration.

This patch worksarounds the issue by providing a hook in URI which allows
clients to provide their customized include path.

Diff Detail

Event Timeline

hokein created this revision.Apr 9 2018, 2:23 AM
sammccall added inline comments.Apr 9 2018, 6:49 AM
clangd/URI.h
63

Avoid "tries to" which is vague about what cases are failures.
e.g.
Get the preferred spelling of this file for #include, if there is one, e.g. <GL.h>.
Returns the empty string if normal include-shortening based on the path should be used.
Fails if the URI is not valid in the schema.

64

what are "clients"? maybe "URI schemas"?

66

does this include <quotes>? it probably should, if we're allowing schemes to customize how includes are spelled.

69

i'd avoid the name "include path" because it can be confused with a) the set of directories passed to -I and b) the filesystem path to the file to be included.
Suggest includeString or so

109

#included

unittests/clangd/ClangdTests.cpp
986

this relies on ClangdTests and URITests being linked together, which we chose to avoid last time this came up. Just define a scheme here?
(This is one place where putting a field on URI would have been simpler)

hokein updated this revision to Diff 141650.Apr 9 2018, 7:51 AM
hokein marked 3 inline comments as done.

Address review comments.

hokein marked an inline comment as done.Apr 9 2018, 7:54 AM
hokein added inline comments.
clangd/URI.h
66

Yes, the returned string is a literal string quoted with either <> or "".

69

Renamed it to spellingInclude.

unittests/clangd/ClangdTests.cpp
986

I think we probably want a scheme for unittest purpose (we also have one for lit test), but this case only needs a trivial one, added here.

sammccall accepted this revision.Apr 9 2018, 7:57 AM
sammccall added inline comments.
clangd/URI.h
69

nit: I think includeSpelling would be clearer, since include is the adjective. Up to you

This revision is now accepted and ready to land.Apr 9 2018, 7:57 AM
hokein updated this revision to Diff 141654.Apr 9 2018, 8:09 AM
hokein marked an inline comment as done.

includeSpelling

This revision was automatically updated to reflect the committed changes.