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