This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support textEdit in addition to insertText.
ClosedPublic

Authored by kadircet on Aug 8 2018, 7:01 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Aug 8 2018, 7:01 AM
ilya-biryukov added inline comments.Aug 9 2018, 10:23 AM
clangd/CodeComplete.cpp
289 ↗(On Diff #159712)

We shouldn't have duplicate/overlapping fix-its, right? Maybe use std::sort?

291 ↗(On Diff #159712)

use built-in tuples comparison std::tie(X.line, X.column) < std::tie(Y.line, Y.column)?

1057 ↗(On Diff #159712)

Maybe add a comment describing the cases where isValid() is false?
Does it happen when we complete with empty identifier?

1310 ↗(On Diff #159712)

Maybe keep the reserve call? (we could reserve one extra element, but that's fine)

clangd/CodeComplete.h
126 ↗(On Diff #159712)

Could we avoid adding textEdit here?
It's an intermediate representation that is only used in clangd, and we should be able materialize the actual textEdit when converting to LSP.

unittests/clangd/SourceCodeTests.cpp
40 ↗(On Diff #159712)

we convert those uint64_t into int when setting the positions anyway.
Maybe use int here too?

kadircet updated this revision to Diff 160073.Aug 10 2018, 2:21 AM
kadircet marked 5 inline comments as done.
  • Resolve discussions.
kadircet added inline comments.Aug 10 2018, 2:22 AM
clangd/CodeComplete.cpp
1310 ↗(On Diff #159712)

Actually we could have much more than one extra element, not for the current situation but may be in the future when we have more fixit completions.

ilya-biryukov accepted this revision.Aug 10 2018, 5:37 AM

LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)

clangd/CodeComplete.cpp
1310 ↗(On Diff #159712)

We can't have more than one edit adjacent to the completion identifier, right? Otherwise they'll conflict.
It is possible to have multiple edits which are consectutive and the last of them is adjacent to the completion identifier, though. But I don't think we handle those cases here anyway.

But I'm not too worried about leaving out the reserve call either. At the very worst we could waste some memory on a single completion item, but we shouldn't keep too many around at the same time anyway, so feel free to ignore this one.

This revision is now accepted and ready to land.Aug 10 2018, 5:37 AM
kadircet updated this revision to Diff 160303.Aug 13 2018, 1:21 AM
  • Rebase.
  • Resolve discussions.
This revision was automatically updated to reflect the committed changes.