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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for late response.



================
Comment at: clangd/ClangdUnit.cpp:610
+        ParameterInformation Info;
+        OptionalParameterLabel = getOptionalString(*Chunk.Optional);
+        Result.label += OptionalParameterLabel;
----------------
Are we first concatenating the `CodeCompletionString` inside optional chunks and then trying to parse them again here?
Can we extract `ProcessChunks` function and recursively call it with `Chunk.Optional->Chunks`?


================
Comment at: clangd/ClangdUnit.cpp:650
+
+template <class ReturnType, class CodeCompleteConsumerType>
+ReturnType
----------------
rwols wrote:
> Not sure if these template names are according to style.
It's ok. See my comment below, though, it looks like we can avoid making this function template in the first place.


================
Comment at: clangd/ClangdUnit.cpp:652
+ReturnType
+invokeClangAction(PathRef FileName, tooling::CompileCommand Command,
+                  PrecompiledPreamble const *Preamble, StringRef Contents,
----------------
Maybe rename to `invokeCodeCompletion`? `invokeClangAction` seems too generic.


================
Comment at: clangd/ClangdUnit.cpp:705
+  Clang->setCodeCompletionConsumer(
+      new CodeCompleteConsumerType(FrontendOpts.CodeCompleteOpts, Results));
 
----------------
Maybe receive `CodeCompleteConsumer` and not make function template?
We can return whether calling an action succeeded instead.


================
Comment at: test/clangd/signature-help.test:39
+# I'm just putting the questionable result in here now as the expected result.
+# CHECK-DAG: {"label":"bar(float x = 0, int y = 42) -> void","parameters":[{"label":"float x = 0, int y = 42"}]}
+# 
----------------
rwols wrote:
> When there are multiple defaulted parameters after each other, the CK_Optional chunk consists of *all* of those parameters, instead of a CK_Optional chunk per parameter. This might require us to dive into SemaCodeComplete.cpp to fix this. I'm just leaving it as-is right now.
But does `CodeCompletionString` inside `Chunk->Optional` contain those extra parameters as a separate chunk?



https://reviews.llvm.org/D38048