[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the change! Overall LG, mostly NITs about the tests.



================
Comment at: clangd/CodeComplete.cpp:959
   bool Incomplete = false; // Would more be available with a higher limit?
-  llvm::Optional<FuzzyMatcher> Filter;       // Initialized once Sema runs.
-  std::vector<std::string> QueryScopes;      // Initialized once Sema runs.
+  llvm::Optional<FuzzyMatcher> Filter;  // Initialized once Sema runs.
+  std::vector<std::string> QueryScopes; // Initialized once Sema runs.
----------------
NIT: this change is unrelated, maybe remove from the diff?

We can submit it separately without review, since it's a noop formatting cleanup


================
Comment at: clangd/CodeComplete.cpp:1101
   // The bundles are scored and top results are returned, best to worst.
-  std::vector<ScoredBundle>
-  mergeResults(const std::vector<CodeCompletionResult> &SemaResults,
-               const SymbolSlab &IndexResults) {
+  std::vector<ScoredBundle> mergeResults(const SymbolSlab &IndexResults) {
     trace::Span Tracer("Merge and score results");
----------------
Same here: maybe remove from this change and submit separately? 
Again, no review needed, this is clearly a no-op change


================
Comment at: clangd/CodeComplete.cpp:1288
+  LSP.additionalTextEdits.reserve(FixIts.size() + (HeaderInsertion ? 1 : 0));
+  for (const auto &FixIt : FixIts)
+    LSP.additionalTextEdits.push_back(FixIt);
----------------
IIUC, it does not play nicely with the insertText in VSCode, right? (I.e. completing `this.^` will produce `this-push_back`)
Let's add a `FIXME` comment explaining this case and indicate how we're going to fix this (using `textEdit` instead of the `insertText`, right?)


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:480
   auto Results = completions("int main() { abs^ }", {ns("absl"), func("absb")});
-  EXPECT_THAT(Results.Completions, HasSubsequence(Named("absb"), Named("absl")));
+  EXPECT_THAT(Results.Completions,
+              HasSubsequence(Named("absb"), Named("absl")));
----------------
NIT: also submit as a separate change?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1354
+          void MemberFunction();
+          Auxilary* operator->() const { return Aux; }
+         private:
----------------
Maybe remove the body of `opeartor->()` and the private field?
Will make the test a little smaller.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1358
+        };
+        namespace ns {
+          void f() {
----------------
Maybe remove `namespace` and put everything into global ns? 


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1361
+            ClassWithPtr x;
+            x->MemberFunction^;
+          }
----------------
Maybe tests without the prefix, so that both AuxFunction and MemberFunction are available and assert that AuxFunction does not have a fix-it?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1369
+  TextEdit ReplacementEdit;
+  ReplacementEdit.range.start.line = 15;
+  ReplacementEdit.range.start.character = 13;
----------------
This can be made more readable with helpers from `unittests/clangd/Annotations.h`.

```
Annotations Source(R"cpp(
/// ....
void f() {
  ClassWithPtr x;
  x[[->]]MemberFunction^ 
})cpp");

Source.point(); // <-- completion point
Source.range(); // <-- range for the fix-it
```

Our `completions` helper already has parsing of annotations internally, though. But we can write a new overload for the helper that accepts completion point and the parsed text directly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50193