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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
