[PATCH] D38048: [clangd] Add textDocument/signatureHelp

[Ilya Biryukov via Phabricator via cfe-commits]

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