Page MenuHomePhabricator

[clangd] Merge results from static/dynamic index.
ClosedPublic

Authored by sammccall on Jan 14 2018, 11:43 PM.

Details

Summary

We now hide the static/dynamic split from the code completion, behind a
new implementation of the SymbolIndex interface. This will reduce the
complexity of the sema/index merging that needs to be done by
CodeComplete, at a fairly small cost in flexibility.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Jan 14 2018, 11:43 PM
hokein added inline comments.Jan 15 2018, 1:21 AM
clangd/index/Merge.cpp
29 ↗(On Diff #129804)

Maybe add another FIXME, saying filer out symbols in static index, but are removed in dynamic index?

34 ↗(On Diff #129804)

IIUC, Dyn is a local variable, which holds all the underlying data of Symbol, the Symbol returned in the callback of fuzzyFind would be invalid after the fuzzyFind(..) function call is finished.

Our previous assumption is that Symbol is valid as long as the SymbolIndex exists?

75 ↗(On Diff #129804)

Don't we need CompletionDetail?

sammccall updated this revision to Diff 129824.Jan 15 2018, 3:02 AM
sammccall marked 2 inline comments as done.

Fixed bug where we wrote into the underlying index's symbols.
Extended testcase.
Added documentation around contracts.

sammccall added inline comments.Jan 15 2018, 3:18 AM
clangd/index/Merge.cpp
29 ↗(On Diff #129804)

Done (function-level comment, because it's not a localized fix)

34 ↗(On Diff #129804)

Our previous assumption is that Symbol is valid as long as the SymbolIndex exists?

No, it's only valid for the callback. Valid for the life of the index would make life hard for remote indexes.
Added documentation to SymbolIndex.

75 ↗(On Diff #129804)

Sure. I was assuming it wasn't optional (should be present and match if USR matches), but i'm not 100% sure.

75 ↗(On Diff #129804)

This was a bug. It'd be caught by marking Symbol::Detail const. Added a TODO.

hokein accepted this revision.Jan 15 2018, 4:26 AM

looks good.

clangd/index/Merge.cpp
34 ↗(On Diff #129804)

I see, that makes sense.

This revision is now accepted and ready to land.Jan 15 2018, 4:26 AM
This revision was automatically updated to reflect the committed changes.