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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

Sorry for delay, I got distracted with other stuff. I addressed your comment, partially, and also added more tests and fixed one more issue (see the FunctionType test, it would've failed before).



================
Comment at: clang/lib/Sema/SemaOverload.cpp:6460
   // list (8.3.5).
   if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
       !Proto->isVariadic()) {
----------------
sammccall wrote:
> Hmm, there are something like 6 callers of TooManyArguments.
> They all check isVariadic, and probably should all be adjusted in this way (except in once case where PartialOverloading is constant `false`).
> This includes one almost-identical caller in this file, for AddMethodCandidate...
> 
> It's tempting to say that we should just pass some more args into TooManyArguments and have it do this logic internally. Maybe pass the FunctionType and the FunctionDecl* in, with the latter nullable?
> At least this would keep this nice comment in one place.
TooManyArguments is called in 5 places:
- Here
- Below, in the method version, which is identical to this case
- In ProduceCallSignatureHelp, where function can be variadic, but it cannot be a template, so no change needed and we don't even have FunctionDecl (just FunctionType)
- in checkDirectCallValidity, where PartialOverloading is unconditionally false, none of this matters and we shouldn't be checking isTemplateVariadic()
- in DeduceTemplateArguments, where we do check isTemplateVariadic() already, but we shouldn't be looking at template instantiation pattern.

additionally there is mergeCandidatesWithResult, where we don't call TooManyArguments, but need this logic too. If we did call TooManyArguments, we'd have to pass PartialOverloading = false, but still check for template variadic stuff.

We only need the pattern logic in 2 out of 5 places and, considering how specific to code completion/sig help it is, and how weird it is too, I'm leaning towards having this be a separate function.

I renamed the helper to something more appropriate and added a call to it in the method case. Let me know if this is better, or if you think I definitely should put this in TooManyArguments() (I'm leaning towards this solution, but don't have a strong opinion on this myself).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111318