[PATCH] D111318: [clang][clangd] Improve signature help for variadic functions.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

Added a slightly awkward clang (not clangd) test. The output is suboptimial, because it's not clangd, but it shows things work as they should.



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:929
+        //
+        // This only matters for variadic functions/templates, where CurrentArg
+        // might be higher than the number of parameters. When that happens, we
----------------
sammccall wrote:
> hmm, now that I think about it, should this conversion from arg-index to param-index be happening here, or done already by sema and the param index passed to ProcessOverloadCandidates?
> 
> (The docs say "current arg" but I'm struggling to see how that can really be useful)
Since ProcessOverloadCandidates is called with multiple candidates CurrentParam wouldn't make sense - which candidate would that be referring to? What if the candidates are:
  void fun1(int x, ...);
  void fun2(int x, int y, ...);
and CurrentArg is 3, that means for the first signature activeParameter = 1, while for the second one activeParameter = 2.

Right now we do not return this information, but we should (I left a FIXME and intend to follow up, LSP already has the per-signature field).

We could consider moving the CurrentArg out of this function signature and into the OverloadCandidate, so it's per candidate in Sema too. Is that what you're suggesting? I think the current state of things is fine, but if you think I should remove CurrentArg and replace it with a field in OverloadCandidate, let me know.



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:945
+        }
+        SigHelp.activeParameter =
+            std::min(SigHelp.activeParameter, NumParams - 1);
----------------
sammccall wrote:
> hmm, if foo() is a non-variadic 0-arg function, we set activeParameter to -1.
> Before this patch it's zero which is still weird, but less weird.
Right, that's unintended. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111318/new/

https://reviews.llvm.org/D111318