This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement a smart version of HeaderSource switch.
ClosedPublic

Authored by hokein on Sep 23 2019, 4:39 AM.

Details

Summary

This patch implements another version header-source switch by incorporating the
AST and index, it will be used:

  • to improve the current header-source switch feature (layer with the existing file heuristic);
  • by the incoming define-outline code action;

Event Timeline

hokein created this revision.Sep 23 2019, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 4:39 AM
hokein updated this revision to Diff 221288.Sep 23 2019, 4:40 AM

Cleanup.

Harbormaster completed remote builds in B38419: Diff 221288.
kadircet added inline comments.Sep 23 2019, 5:08 AM
clang-tools-extra/clangd/HeaderSourceSwitch.cpp
22

this class doesn't seem to have any state(apart from saving AST in constructor and using it in call to collect), why not just have a vector<Decl> collectDecls(ParsedAST& AST); ?

62

It seems like we have a bunch of different implementations for this function in background.cpp, codecomplete.cpp, ... Basically any place calling URI::resolve, do you mind adding an overload for URI::resolve that'll take a const char* instead of a URI?

79

As discussed in the design for define out-of-line, this function's dependency on AST and Index should rather be optional.

  • if we have AST, then we can boost the files containing the canonical declarations for symbols defined in the main file.
  • if we have just the Index, currently there's nothing much we can do, but we can put a fixme to add a new endpoint to symbol index for "fetching symbols in a given file", then we can use symbol information for declaration/definition files without an AST.
  • if we have AST+Index, we can boost the files containing the definition for symbols declared in the main file and vice-versa(what you already do in this patch).
  • If we don't have anything we'll just make use of the filename, by changing the extension, as current ClangdServer::switchSourceHeader implementation does.
hokein updated this revision to Diff 221486.Sep 24 2019, 2:07 AM
hokein marked 4 inline comments as done.

Address comments.

hokein added inline comments.Sep 24 2019, 2:07 AM
clang-tools-extra/clangd/HeaderSourceSwitch.cpp
22

yeah, but we need to do recursive calls, which I thought making a class is more readable. Change to a single function with recursive lambda, I hope it won't hurt the code readability.

62

Fair suggestion, done in r372617.

79

as discussed offline, we will

  • make the filename heuristic into a separate API which provides more flexible;
  • make this API support 3 cases (except the case where we only have index, as it'd require large effort to implement "fetching symbols in a given file" in the index, we're less certain it is worth); when we only have AST (or the index doesn't contain any interesting information), we just use the information for the AST to do the ".cc=>.h" inference.

mostly LG, a few small comments

clang-tools-extra/clangd/HeaderSourceSwitch.cpp
86

should we have a cache for these uri resolutions?
We will most likely have a few URIs that are being resolved over and over again.

114

what if there are ties ?

we don't need to do anything clever yet, but we should at least be deterministic. Currently it depends on stringmap ordering.
Could you pick the candidate that comes first in the lexical order in such cases?
and add some test cases?

clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
141

no need for raw string literals?

158

maybe just const std::string ? testpath is returning a value, not a ref.

160

maybe also set TU.FileName to TestFilePath ?

196

again no need for raw literals

217

same as above

219

again set TU.FileName ?

hokein updated this revision to Diff 222111.Sep 27 2019, 2:44 AM
hokein marked 9 inline comments as done.

address review comments.

hokein updated this revision to Diff 222112.Sep 27 2019, 2:46 AM

format the testcases.

clang-tools-extra/clangd/HeaderSourceSwitch.cpp
86

We will most likely have a few URIs that are being resolved over and over again.

This is true, but having a cache seems to be a premature optimization at the moment. I think the number for the Decl in the main file is relatively small in practice. We could add it when it turns out a performance problem.

114

good point, I thought the StringMap is sorted by the key value, but it uses hash value.

Harbormaster completed remote builds in B38654: Diff 222112.
kadircet accepted this revision.Sep 27 2019, 9:23 AM

thanks for formatting test cases, I had it in mind but forgot to mention in last round.

LGTM

clang-tools-extra/clangd/HeaderSourceSwitch.cpp
113

nit: for(auto &It : Candidates)

117

nit: else if

clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
196

not done

This revision is now accepted and ready to land.Sep 27 2019, 9:23 AM
hokein updated this revision to Diff 222360.Sep 30 2019, 12:57 AM
hokein marked 3 inline comments as done.

address review comments.

clang-tools-extra/clangd/HeaderSourceSwitch.cpp
113

for-range doesn't work here, the type is StringMapEntry which is a non-copy structure, we need to copy an iterator for recording the Best.

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