[PATCH] D50449: [clangd] Support textEdit in addition to insertText.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/CodeComplete.cpp:289
       }
+      std::stable_sort(Completion.FixIts.begin(), Completion.FixIts.end(),
+                       [](const TextEdit &X, const TextEdit &Y) {
----------------
We shouldn't have duplicate/overlapping fix-its, right? Maybe use `std::sort`?


================
Comment at: clangd/CodeComplete.cpp:291
+                       [](const TextEdit &X, const TextEdit &Y) {
+                         return X.range.start.line < Y.range.start.line ||
+                                X.range.start.character <
----------------
use built-in tuples comparison `std::tie(X.line, X.column) < std::tie(Y.line, Y.column)`?


================
Comment at: clangd/CodeComplete.cpp:1057
+    Range TextEditRange;
+    if (CodeCompletionRange.isValid()) {
+      TextEditRange = halfOpenToRange(Recorder->CCSema->getSourceManager(),
----------------
Maybe add a comment describing the cases where `isValid()` is false?
Does it happen when we complete with empty identifier?


================
Comment at: clangd/CodeComplete.cpp:1310
+  // other.
+  for (const auto &FixIt : FixIts) {
+    if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {
----------------
Maybe keep the `reserve` call? (we could reserve one extra element, but that's fine)


================
Comment at: clangd/CodeComplete.h:126
 
+  TextEdit textEdit;
+
----------------
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.


================
Comment at: unittests/clangd/SourceCodeTests.cpp:40
 
+Range range(const std::pair<uint64_t, uint64_t> p1,
+            const std::pair<uint64_t, uint64_t> p2) {
----------------
we convert those `uint64_t` into `int` when setting the positions anyway.
Maybe use `int` here too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50449