This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add code completion support
ClosedPublic

Authored by krasimir on Mar 24 2017, 6:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

krasimir created this revision.Mar 24 2017, 6:18 AM
krasimir updated this revision to Diff 92931.Mar 24 2017, 6:30 AM
  • Clean-up leftovers
bkramer added inline comments.Mar 24 2017, 7:01 AM
clangd/ASTManager.cpp
264 ↗(On Diff #92930)

CompletionItem::unparse should do the escaping. It's weird to have YAML stuff here.

267 ↗(On Diff #92930)

std::move

276 ↗(On Diff #92930)

I think returning by reference and moving out of CodeCompletionInfo in codeComplete() is safe.

clangd/ASTManager.h
78 ↗(On Diff #92931)

Some documentation on the lock would be useful.

I'm a bit worried about the priorities here, code completion should always have priority over parsing for diagnostics. But we can address that later.

clangd/Protocol.cpp
635 ↗(On Diff #92930)

Os.flush();

640 ↗(On Diff #92930)

Os.flush();

clangd/Protocol.h
289 ↗(On Diff #92930)

Just use C++11 inline initialization, no need to spell out the constructor.

krasimir updated this revision to Diff 92939.Mar 24 2017, 7:06 AM
  • Add '.' and '>' as completion trigger characters
krasimir updated this revision to Diff 92940.Mar 24 2017, 7:26 AM
krasimir marked 7 inline comments as done.
  • Address review comments

Urg, I was working on this too :) I'll compare implementations and provide comments if I find anything good to suggest.

krasimir updated this revision to Diff 92948.Mar 24 2017, 7:59 AM
  • Make Items ownership more explicit
test/clangd/formatting.test
14 ↗(On Diff #92948)

It would be good eventually to trigger on "::" (static members, etc)

Ideas/Observations:

  • One thing I has done in my version is to introduce "ASTUnitRunnable", a lambda function type that has an ASTUnit as parameter and is executed by the ASTManager. So the ASTManager takes care of locking the AST but the actual code using the AST is kept in the ProtocolHandlers. I think this can be refactored later if this is seen as a good approach.
  • I have noticed while testing that there is some wrong escaping happening. I had typed "ù" by mistake (french keyboard layout!) and it threw an error about unexpected \x. I think using yaml::escape is not entirely correct. This can be addressed in a different patch because it affects other protocol handlers as well (diagnostics, etc).
  • I noticed that "file://" is stripped from file:///path but not "file:" in file:/path, I think it's the Eclipse client that's wrong here.

But I don't see any immediate need to change anything in this patch.

This revision is now accepted and ready to land.Mar 28 2017, 4:11 PM
stanionascu added inline comments.
clangd/Protocol.cpp
613 ↗(On Diff #92948)

if kind is actually provided there will be a trailing quote, as in ("kind": 4").

This revision was automatically updated to reflect the committed changes.