[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! The only major comment I have is about the `FixItsScore`, others are mostly NITs.



================
Comment at: clangd/Quality.cpp:298
+    FixItCount += FixIt.CodeToInsert.size();
+  }
 }
----------------
NIT: remove braces around single-statement loop body


================
Comment at: clangd/Quality.h:80
   bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private).
+  int FixItCount = 0; // Number of additional changes that needs to be performed
+                      // for this change to take place.
----------------
The comment is a bit misleading, since it's actually the length of the inserted code.
I'd suggest keeping the comment and a name of this variable a bit more vague on the definition of this field, e.g. something like

`int FixItScore; // Measure of the total amount of edit needed by the fix-its. Higher is worse, i.e. more code needs to change`

And overall, it looks like the penalty might be too harsh for some potential fix-its. E.g. imagine a fix-it that adds a `using ns::some_class` directive somewhere to the file. The size of the inserted text will be proprotional to the size of the fully-qualfiied-name of the class, but it does not seem like the fix-its for items with larger names should be downranked more than others, after all it's the same type of fix-it.

```
// Hypothetical example, we don't have fix-its like that yet.
namespace ns {
  class SomeVeryLongName {};
  class ShortName {};
}


^ // <-- complete here, imagine the completion items for both classes are available and 
  // add `using ns::<ClassName>;`  to the global namespace.
```

In that case, we will penalize `SomeVeryLongName` more than `ShortName`, because it has a longer fix-it, but it does not look like a useful distinction.
I suggest we start with something simple, like having the same predefined penalty for all fix-its or penalize based on the number of fix-its in the list. We can generalize later when we see more fix-its and have a better understanding on penalties we want from them


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:112
+
+CodeCompleteResult completions(ClangdServer &Server, Annotations Test,
+                               std::vector<Symbol> IndexSymbols = {},
----------------
NIT: use `const Annotations&`?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:119
+
+CodeCompleteResult completions(ClangdServer &Server, StringRef Text,
+                               std::vector<Symbol> IndexSymbols = {},
----------------
The number of `completions` overloads seems to be getting out of hand.
Any ideas on how we can reduce them?

Generally, I think we want the most general version:
```
CodeCompleteResult completionsImpl(ClangdServer&, Text, Point, Index, Opts)
```
And two conveniece wrappers that create `ClangdServer` for us:
```
// Parses Annotations(Text) and creates a ClangdServer instance.
CodeCompleteResult completions(Text, Index = ..., Opts = ...);
// Only creates a ClangdServer instance.
CodeCompleteResult completions(Text, Point, Index=..., Opts =...);
```

Can we make the rest of the code to use this API? (e.g. if function accepts a `ClangdServer`, the callers have to do quite a bit of setup anyway and I don't think adding the annotations parsing to get the completion point will hurt the readability  there). 


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1397
+    EXPECT_TRUE(C.FixIts.size() == 1u || C.Name == "AuxFunction");
+    if (!C.FixIts.empty()) {
+      EXPECT_THAT(C.FixIts, ElementsAre(ReplacementEdit));
----------------
NIT: remove braces around single-statement ifs, LLVM code generally tends to avoid adding braces around single-statement bodies of loops, conditionals, etc


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1439
+  CodeCompleteOptions Opts;
+  Opts.IncludeFixIts = true;
+  StringRef TestCode(
----------------
We tend to unit-test the scores separately from general completion tests.
Could we replace this test with a unit-test in `QualityTests.cpp`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50193