This is an archive of the discontinued LLVM Phabricator instance.

[clangd] codeComplete returns more structured completion items, LSP. NFC.
ClosedPublic

Authored by sammccall on Jun 29 2018, 3:34 AM.

Details

Summary

LSP has some presentational fields with limited semantics (e.g. 'detail') and
doesn't provide a good place to return information like namespace.

Some places where more detailed information is useful:

  • tools like quality analysis
  • alternate frontends that aren't strictly LSP
  • code completion unit tests

In this patch, ClangdServer::codeComplete still return LSP CompletionList, but
I plan to switch that soon (should be a no-op for ClangdLSPServer).

Deferring this makes it clear that we don't change behavior (tests stay the
same) and also keeps the API break to a small patch which can be reverted.

Event Timeline

sammccall created this revision.Jun 29 2018, 3:34 AM

@jkorous FYI: this may be interesting from an XPC perspective - it'd be easy to serialize the LSP-style struct, or the more detailed one if that's useful.
LSP will be more stable of course :-)

Looks great! Mostly nits.

clangd/CodeComplete.cpp
259

nit: shall we keep assert(bool(SemaResult) == bool(SemaCCS));?

279

remove?

280

Inserted can be an absolute path which might have long uninteresting prefix. It might make sense to use the result from calculateIncludePath?

309

nit: It's unclear why add is a public interface given that the constructor also adds candidate. Might worth a comment. Also, it seems that the interface design has been driven by by the bundling logic, so it might also make sense to explain a bit on the class level.

322

nit: the documentation for the bundling says Documentation may contain a combined docstring from all comments. The implementation seems to take the documentation from the last comment (which seems fine to me), but they should be consistent.

346

nit: this might worth a comment. it's unclear what this does by just looking at the name.

1145

nit: this seems to assume that NameMatch is a factor of the final score, which seems to be changeable given the abstraction of evaluateSymbolAndRelevance. Not sure if there is an alternative to the current approach, but this probably deserves a comment.

1161

nit: toCompletionItem might be ambiguous now that this returns CodeCompletion.

clangd/CodeComplete.h
88

Give that this field has caused confusion before, would it make sense to make the name a bit more concrete like RequiredQualifier or InsertedQualifier?

106

Would this be an absolute path or an include path?

140

I found Result.More a bit not straightforward. Maybe HasMore?

sammccall updated this revision to Diff 153493.Jun 29 2018, 7:36 AM
sammccall marked 11 inline comments as done.

Address review comments.

clangd/CodeComplete.cpp
259

Done (in add(), and hoisted the call to add() to the top here)

322

This was meant to be ambiguous ("may") to allow a better implementation later. But rewrote it to mention the current behavior as a possibility :-)

1145

Yeah, client-side filtering is the great abstraction-breaker :-(
Added the comment, not sure there's much more to be done.

ioeric accepted this revision.Jun 29 2018, 7:41 AM

lgtm

This revision is now accepted and ready to land.Jun 29 2018, 7:41 AM
This revision was automatically updated to reflect the committed changes.