ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Thanks for the patch. Overall looks good, just a few comments.
Could you also add some tests?
================
Comment at: clangd/ClangdServer.h:243
+ /// Provide signature help for \p File at \p Pos. If \p OverridenContents is
+ /// not None, they will used only for code completion, i.e. no diagnostics
+ /// update will be scheduled and a draft for \p File will not be updated. If
----------------
Mentions "code completion" in the comment.
================
Comment at: clangd/ClangdUnit.cpp:536
+ SigHelp.signatures.reserve(NumCandidates);
+ // TODO(rwols): How can we determine the "active overload candidate"?
+ SigHelp.activeSignature = 0;
----------------
Code in clangd uses `FIXME` instead of `TODO`.
================
Comment at: clangd/ClangdUnit.cpp:589
+ }
+ case CodeCompletionString::CK_Optional:
+ case CodeCompletionString::CK_VerticalSpace:
----------------
Ignoring `CK_Optional` chunks leads to weird results. They represent parameters
that have default arguments. For example,
```
int foo(int a, int b = 10, int c = 20);
int test() {
foo(10, 20, 30); // signatureHelp shows foo(int a) here, which is confusing.
}
```
================
Comment at: clangd/ClangdUnit.cpp:691
+ std::shared_ptr<PCHContainerOperations> PCHs) {
+ std::vector<const char *> ArgStrs;
+ for (const auto &S : Command.CommandLine)
----------------
Most of this function is almost identical to `clangd::codeComplete`. They only
differ in some of the `CodeCompleteOpts` flags and `CodeCompletionConsumer`s
they use, right?
Maybe extract a helper function that runs code completion and reuse it for both
`clangd::codeComplete`and `clangd::signatureHelp`?
https://reviews.llvm.org/D38048
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
