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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
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
----------------
adamcz wrote:
> 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.
> 
Whoops, of course you're right, it's more complicated.

In principle, this seems like it does belong on OverloadCandidate. Doing this as a field seems like a yak-shave, maybe it makes sense to make paramIndexForArg a method on OverloadCandidate though. (And still require CurrentArg as input). Up to you, this is fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111318