Page MenuHomePhabricator

[clangd] Use identifiers in file as completion candidates when build is not ready.

Authored by ioeric on Apr 2 2019, 6:28 AM.



o Lex the code to get the identifiers and put them into a "symbol" index.
o Adds a new completion mode without compilation/sema into code completion workflow.
o Make IncludeInserter work even when no compile command is present, by avoiding
inserting non-verbatim headers.

Diff Detail

Event Timeline

ioeric created this revision.Apr 2 2019, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 6:28 AM
sammccall added inline comments.Apr 4 2019, 5:28 AM

as discussed offline, I love the lexer approach but returning it as an index seems like the wrong level of abstraction.
These are just strings, and if we want to tweak e.g. the scoring or merging of these we'll be in a better position to do so if we don't pretend they have Symbol semantics (their own signals, ids, etc).
So I'd suggest this fn should return something like StringSet<> or StringMap<unsigned> for counts.

Can we move it to SourceCode.h too?


I don't think we need FileName as a parameter - it shouldn't affect the return value, so we can just use a dummy value.


I'm not sure these changes (mostly to comment formatting) improve clarity.
I think it would be enough to add a comment inside the top of runWithoutCompile saying // Fill in fields normally set by runWithSema() or so




runWithoutSema maybe? - compile is a fine name but this file calls this concept sema for whatever reason


hmm, this function should likely also move to SourceCode, use Offset instead of Position, and move to SourceCode.h. But probably a different patch.


you could just erase the typed filter from the suggestion list.
(It may be a valid word spelled elsewhere, but there's no point suggesting it)


I suspect you want to pass the filtered strings through to mergeResults and have it/CompletionCandidate do the scoring.


It seems a little odd to have a different entrypoint here.
Could we simply indicate this with Preamble=nullptr?

It seems it might even make sense to use the same speculative fuzzyfind?
So I think PCHContainerOperations would be the only odd one out. I'm going to send a patch to remove that anyway :-)

ioeric updated this revision to Diff 194311.Apr 9 2019, 6:42 AM
ioeric marked 9 inline comments as done.
  • address review comments
ioeric updated this revision to Diff 194315.Apr 9 2019, 6:49 AM
  • minor cleanup
ioeric added a comment.Apr 9 2019, 6:49 AM

Thanks for the review!




This is following conventions of other sources. Both index and sema provide results for fully-typed names. Considering that users might be already used to this, and completion results tend to give reassurance for correctly typed names, I am inclined to keep the typed filter if it's seen somewhere else in the file.

Very nice! D60503 will conflict, feel free to stall that until this is landed.
On the other hand it will simplify some things, e.g. the prefix is already calculated, and typed scope is available if you want that (no enclosing namespaces though).


This is a bit vague - most results are for identifiers in some sense.
Maybe RawIdentifier?


move this comment to the member?
The invariant is important to readers, not just callers of the constructor.


rename NBoth -> NSemaAndIndex?


it doesn't make sense to set this here, as long as we're not querying the index.


OK, but we should now be able to just decrement the count for the right entry in the collected identifiers then? rather than carving up the string


add this to the non-sema case too (though the if is always false for now), or add a fixme?
Worried about forgetting this.


why are we not initializing fileproximity in the fallback case?
(To only a single source)


can we have a SymbolOrigin bit for identifiers, and use that instead?


nit: i'd move this above the previous case, it seems both more fundamental and cheaper


Do we expect this code path to ever be called?
We may want to assert or elog here, depending on how this can be hit.


why isn't Add just ++Identifiers[Id]?

ioeric updated this revision to Diff 194647.Apr 11 2019, 1:32 AM
ioeric marked 14 inline comments as done.
  • address review comments

Added a FIXME.


This can be hit when we start serving index symbols in fallback mode completion. shouldInsertInclude will return false, but the calculation will be attempted regardless. We can probably elog this, but the number of log messages would be O(# of index symbols), which I worry might be too spammy.

sammccall accepted this revision.Apr 11 2019, 2:26 AM
sammccall added inline comments.

Completion.Kind = CompletionItemKind::Text


comment should explain why, rather than what :-)

This revision is now accepted and ready to land.Apr 11 2019, 2:26 AM
ioeric updated this revision to Diff 194652.Apr 11 2019, 2:31 AM
ioeric marked 2 inline comments as done.
  • address comments
This revision was automatically updated to reflect the committed changes.