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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
