sammccall added a comment. Thanks, this looks nice!
Since the code is in Sema, we'd should (also) test this in `clang/test/CodeCompletion/` if possible. ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:928 + // FIXME: Add support for per-signature activeParameter field. + // + // This only matters for variadic functions/templates, where CurrentArg ---------------- I'd consider pulling out the logic into a helper named something like `paramIndexForArg(int, OverloadCandidate)` just because this function is ridiculously long and this piece of logic encapsulates nicely ================ 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 ---------------- 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) ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:945 + } + SigHelp.activeParameter = + std::min(SigHelp.activeParameter, NumParams - 1); ---------------- 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. 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
