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