Implementation for completion in call context for C++

[francisco.lopes]

Summary

Improved completion in call context to work with:

• functions
• member functions
• constructors
• new expressions
• function call expressions
• template variants of the previous

[francisco.lopes]

Basic functor parameter completion was added.

It's currently working for:

struct S { void operator()(int){} };
int main() {
    S s;
    s(>|<);
}

but not for:

struct S { void operator()(int){} };
int main() {
    S{}(>|<);
}

[klimek]

+Richard, Doug, who needs to sign off on changes to code completion behavior?

[francisco.lopes]

I expect to do some refactoring to reduce code duplication if possible in the following days, and maybe get the extended support for constructors, etc. The current state can be merged now without problems, since the current behavior will be the same after I do the remaining changes.

[francisco.lopes]

I can do the merge myself, in case I get a review.

[francisco.lopes]

Good amount of refactoring. Now providing extend support for function call operator.

[klimek]

+chandler, since nobody else gave any pointers at who needs to approve this

[francisco.lopes]

Support for constructors and new expressions.

[francisco.lopes]

@klimek Hi, I'm providing differential diffs, not sure whether they should have been accumulative. I think it's in good enough shape now, I don't expect to tweak it for a while.

[klimek]

As I said, we'll first need to find somebody who knows the trade-offs of that part of the code base to be able to sign off on the high level functionality change - did you already send an email with that to cfe-dev by the way? That might be the better venue for a high level discussion of the feature...

[francisco.lopes]

As per conversation at cfe-dev, I've agreed it's best for now to have the original ordinary name code-completion behavior preserved while providing the new set of overload candidates completions.

I had to create an additional CXCursorKind named CXCursor_OverloadCandidate, so that there's any way to discern overload candidates results.

Futurely there may be an API with specific purpose of querying for overload candidates solely, I won't implement it for now.

The tests that were deprecated before were added back.

[francisco.lopes]

If I'm not mistaken I should have been using Arcanist erroneously by submitting relative diffs.

This patch is being produced solely to provide an unified diff.

[klimek]

General high level comment: you're using C++11 extensively, have you made sure you're only using features MSVC 2012 supports (as per the style guide: http://llvm.org/docs/CodingStandards.html)?

[francisco.lopes]

Thanks for the feedback Manuel,

I didn't try it in MSVC 2012, I hope it's not a hassle to compile clang in
Windows,
I'm downloading the express edition so I can try it.

I believe the sole critical piece regarding the style guide is the
declaration of the
ParseExpressionList function. Although I think it's good to have the
default argument
initialized that way, which is simple and short, it may not be in
accordance with the
"Do not use Braced Initializer Lists to Call a Constructor" section of the
style guide.

As I'm unsure about this specific usage, I'll left it as is until proper
review.

2015-01-13 17:12 GMT-02:00 Manuel Klimek <[e-mail]>:

General high level comment: you're using C++11 extensively, have you made
sure you're only using features MSVC 2012 supports (as per the style guide:
http://llvm.org/docs/CodingStandards.html)?

http://reviews.llvm.org/D6880

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[francisco.lopes]

In D6880#108168, @francisco.lopes wrote:

Thanks for the feedback Manuel,

I didn't try it in MSVC 2012, I hope it's not a hassle to compile clang in
Windows,
I'm downloading the express edition so I can try it.

I believe the sole critical piece regarding the style guide is the
declaration of the
ParseExpressionList function. Although I think it's good to have the
default argument
initialized that way, which is simple and short, it may not be in
accordance with the
"Do not use Braced Initializer Lists to Call a Constructor" section of the
style guide.

As I'm unsure about this specific usage, I'll left it as is until proper
review.

2015-01-13 17:12 GMT-02:00 Manuel Klimek <[e-mail]>:

General high level comment: you're using C++11 extensively, have you made
sure you're only using features MSVC 2012 supports (as per the style guide:
http://llvm.org/docs/CodingStandards.html)?

http://reviews.llvm.org/D6880

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

Yeah, MSVC 2012 doesn't like copy-list-initialization either. Fixed, thanks again for the feedback.

[klimek]

lib/Sema/SemaOverload.cpp

5854

Indent.

5860–5862

Here and elsewhere: this looks like it actually changes overload resolution, but I didn't see a description about that change in the change comment - what am I missing?

5968–5969

This is a little too clever for me :)
a) comment on why we need to +1 when PartialOverloading
b) pull it out of the if into a variable?

[francisco.lopes]

lib/Sema/SemaOverload.cpp

5860–5862

This change is not required in truth (originally I was using it in CodeCompleteConstructor too), I've left it for a good reason. AddFunctionCandidates looks like a function that was added much later than the ones it calls, and it's being called in few places in the codebase, none involving constructors, hence no issue has been raised before. If you check inside AddMethodCandidate there's an assertion telling it shouldn't be called for constructors, and that this is a job for AddOverloadCandidate. The way AddFunctionCandidates was before would do exactly that: if Fns contained a constructor, AddMethodCandidate would be called for them, with also the added risk of accessing Args[0] when Args is empty.

5968–5969

For me too! but I've just copied the behavior from some original place. Changing all references of such snippet would mean to enter refactoring of the original code. While applying these changes I paused for a moment to analyze what the **** this was doing. I think I got it for a moment and didn't care anymore and just replicated where it was necessary.

Do you think this is time for refactoring the changeset and the remaining references of this
code?

[klimek]

lib/Sema/SemaOverload.cpp

5860–5862

If it's not required, could you perhaps split up the CL into 2 changes:

1. the code completion change
2. the overload resolution change

That would make it much easier for me to review the change and make sure each part has sufficient tests.

5968–5969

You're right, now is not the time for a refactoring.

[francisco.lopes]

lib/Sema/SemaOverload.cpp

5860–5862

What you have in mind for how do this the best way? I'm not sure how to proceed, I'm thinking of doing a rebase that would take this overload resolution change from the middle of the commit history and push it to the tip as a commit of its own.

[klimek]

lib/Sema/SemaOverload.cpp

5860–5862

Since clang upstream is SVN, all the git history will be lost anyway, so you have multiple options how to do this:
a) rebase
b) create two branches, revert the parts that shouldn't go into that branch manually with a tool like meld, send out two patches

a) is nice if the rebase works without conflicts, otherwise I've done b) before without too many problems.

[francisco.lopes]

Changes suggested by @klimek were applied.

[klimek]

The latest version seems to be exactly the same as the previous version :)

[francisco.lopes]

In D6880#108629, @klimek wrote:

The latest version seems to be exactly the same as the previous version :)

Phabricator's Differential tool doesn't display whitespace changes (I've followed the small indentation suggestion) and also it's not possible to show differences through the commits, only through the patches that are being sent. I've rebased and added a new commit, but the changeset as a whole only changes regarding the indentation. You may check what happened at https://github.com/oblitum/clang/tree/improved_argument_hints. I finding Phabricator awkward to use for these kind of modifications.

[francisco.lopes]

In D6880#108645, @francisco.lopes wrote:

In D6880#108629, @klimek wrote:

The latest version seems to be exactly the same as the previous version :)

Phabricator's Differential tool doesn't display whitespace changes (I've followed the small indentation suggestion) and also it's not possible to show differences through the commits, only through the patches that are being sent. I've rebased and added a new commit, but the changeset as a whole only changes regarding the indentation. You may check what happened at https://github.com/oblitum/clang/tree/improved_argument_hints. I finding Phabricator awkward to use for these kind of modifications.

Notice that in Differential the new commits are being listed, it's just not possible to see what they actually do...

[klimek]

Yes, phabricator doesn't support using a single branch for multiple diffs well - the best way to use it is to create feature branches that each have a single feature...

[francisco.lopes]

Removing the fix that avoids AddFunctionCandidates from calling AddMethodCandidate and AddMethodTemplateCandidate (which it shoudn't) out to a different patch to be applied separately.

[francisco.lopes]

I'll commit this fix separately after this patch gets merged upstream.

[klimek]

Thx for your patience; just to be clear: I think this is a great patch, but I'll need to get it down into chunks I can bite off from as reviewer :)

lib/Sema/SemaOverload.cpp

5968–5970

Ok, sorry for the many questions, but I'm new to this part of the code myself :)
This also changes the overload resolution, right? Is this an integral part of the patch, or could this also be split out? If it cannot be split out, can you explain why we now need this and which test cases apply to that?

[francisco.lopes]

lib/Sema/SemaOverload.cpp

5968–5970

• Yes, this kind of changes that involve the PartialOverloading flag should change the behavior for overloading resolution solely when it's true.

• Yes, it's an integral part of the patch. As can be seen the original codebase already have this kind of check where it's already supporting "partial" overloading. It can't be splitted out.

As can be checked in SemaOverload.cpp:

/// AddOverloadCandidate - Adds the given function to the set of
/// candidate functions, using the given function call arguments.  If
/// @p SuppressUserConversions, then don't allow user-defined
/// conversions via constructors or conversion operators.
///
/// \param PartialOverloading true if we are performing "partial" overloading
/// based on an incomplete set of function arguments. This feature is used by
/// code completion.

PartialOverloading is used in code completion.

The most basic situation to make sense of this code is when code completion is requested in calling context, just after a comma for an argument. Let's say the prototype of the function being called has just one parameter. So in this test NumParams would be 1. As code completion was triggered just after the comma to insert a second argument that was still not inserted, Args.size() is still 1. So Args.size() > NumParams is false but the test must still succeed to set Candidate.FailureKind = ovl_fail_too_few_arguments since the extra comma means a second argument follows.

Notice that even though the code is being correct at this moment for failing with ovl_fail_too_few_arguments, this relates with another part of the code where I do add all candidates that failed with this reason to the result set of viable candidates.

The reason I'm adding the candidates that fail because of "too many arguments" to the result set is because the user will be able to obtain the overloads of that function so it can be used for displaying the expected prototypes, and also it will be able to verify when the overloads are not matching because they won't be providing a "current argument", which all successful overloads with more that one argument will be providing.

Anyway, this last part is still open for discussion although I think a change regarding this coming afterwards won't harm either.

[francisco.lopes]

lib/Sema/SemaOverload.cpp

5968–5970

Correction: which all successful overloads with one parameter or more will be providing.

[francisco.lopes]

In D6880#108666, @klimek wrote:

Thx for your patience; just to be clear: I think this is a great patch, but I'll need to get it down into chunks I can bite off from as reviewer :)

I'm unsure whether you're just generally commenting regarding the incoming questions about the patch, or whether you're requesting to cut the patch somehow for it to be commited in separate chunks.

[francisco.lopes]

Handles force added overloads in similar fashion

The way candidates that failed in overload resolution (for reason of too
many arguments) were being force added wasn't symmetric. This makes this
forced inclusion similar in all cases, which ends up having side-effects
in how such candidates gets sorted. The sorting now looks improved.

I'm really unsure about having this feature. It seems useful at the same time
as it doesn't seem righteous from the point of view of being strict and not giving
any results for extra commas.

[francisco.lopes]

This commit removes the feature I've mentioned before, after having improving it =D

[francisco.lopes]

In D6880#109061, @francisco.lopes wrote:

This commit removes the feature I've mentioned before, after having improving it =D

I've also rebased the changeset over upstream since it has been some time already.

[klimek]

Ok, I think the scope of the patch is now ok - going into detailed code review :)

lib/Sema/SemaCodeComplete.cpp

3852–3858 ↗

(On Diff #18215)

Generally, try to keep the scope of variables as short as possible. Just declare stuff in the loop if it's only used there.

3855–3857 ↗

(On Diff #18215)

Why is it ok to remove the DeclRefExpr case? Where is this handled in the new code?

3859–3861 ↗

(On Diff #18215)

This needs a comment explaining why we do that; why is it a simple lvalue if UnresExpr->isArrow()? Why not use Classify unconditionally?

3862–3863 ↗

(On Diff #18215)

I thought be now we should have a way to use a range based for loop here?
for (auto Decl : UnresExpr->decls()) {

...

}

3877 ↗

(On Diff #18215)

You can declare Method in the if:
} else if (CXXMethodDecl *Method = dyn_cast...

3880 ↗

(On Diff #18215)

I'd just inline UnresExpr->hasExplicitTemplateArgs() here.

3889 ↗

(On Diff #18215)

Any reason to not just use &UnresExpr->getExplicitTemplateArgs()?

3896–3897 ↗

(On Diff #18215)

This needs a comment as to what exactly we're trying to address here...

3902 ↗

(On Diff #18215)

Is the suppressDiagnostics() really needed (I'm not familiar with this, so I just want to make sure :)

3912–3916 ↗

(On Diff #18215)

A short description on what we're going for here would also help tremendously :)

4006–4037 ↗

(On Diff #18215)

This looks copied. Pull out a function.

lib/Sema/SemaOverload.cpp

5967–5970

Looking at this some more (and now understanding the code a bit better, thanks for explaining) I still have the feeling this is wrong.
What happens if you make this (Args.size() > NumParams)? Which tests fail?

This dubious pattern was previously used in a single location; spreading it further is actively harmful, I think, so I'd strongly vote for either making sure we don't need it, or adding comments where it is needed (or a function that is named according to what this does).

[francisco.lopes]

lib/Sema/SemaCodeComplete.cpp

3852–3858 ↗

(On Diff #18215)

OK.

3855–3857 ↗

(On Diff #18215)

This is handled later so that cases with higher priority in the if-else chain gets a chance of being handled. For example, if it's a DeclRefExpr, but getDecl() it's not a FunctionDecl, it could be a CXXRecordDecl that overloads the function-call-operator. So without thinking further in compacting the if-else chain, I've put this case in the end. If you've a suggestion and are sure about how to eliminate cases, I would appreciate. To make sure none of the cases are dead code, I've had run my long series of test cases while debugging this part of the code just to check if all cases are entered, in fact they do.

3859–3861 ↗

(On Diff #18215)

I didn't think about it, I would have to check the docs to make sure since this was copied from elsewhere and I dunno what intent it had formelly.
99% of this section inside if (auto UnresExpr = dyn_cast<UnresolvedMemberExpr>(NakedFn)) was copied verbatim from elsewhere in the codebase. As I'm very new to it (before starting this patch I've read the CFE internals manual for the first time and got to work) you may feel free to guide me what constructions would be much more clearer if using some API, etc. I'll try it and check whether the tests are OK.

3862–3863 ↗

(On Diff #18215)

iterators have getPair(), which is used inside the loop, so the range-for may not be an option. This made me notice I is of type UnresolvedSetIterator, which made me realize this section could be entirely refactored based on a snippet of code I've produced myself.

3877 ↗

(On Diff #18215)

Thanks, this will not apply anymore.

3880 ↗

(On Diff #18215)

Thanks, this will not apply anymore.

3889 ↗

(On Diff #18215)

Thanks, this will not apply anymore.

3896–3897 ↗

(On Diff #18215)

OK.

3902 ↗

(On Diff #18215)

I'm not sure, will check after the first batch of refactorings.

3912–3916 ↗

(On Diff #18215)

OK.

4006–4037 ↗

(On Diff #18215)

OK.

lib/Sema/SemaOverload.cpp

5967–5970

It's necessary, for example, using solely Args.size() > NumParams in the former original location leads to test/CodeCompletion/call.cpp to fail with the following:

<stdin>:44:19: error: CHECK-CC2-NOT: string occurred!
OVERLOAD: [#void#]f(N::Y y, int ZZ)
                  ^
/opt/src/llvm/tools/clang/test/CodeCompletion/call.cpp:26:20: note: CHECK-CC2-NOT: pattern specified here
 // CHECK-CC2-NOT: f(N::Y y, int ZZ)

The reason for it to fail follows exacly from my explanation. It's interesting that there isn't much usage of negative checks in the tests, so I coudn't think of using them myself.

The only option is to refactor the test, it was not used elsewhere because support for completion in call context was minimal, as also the occurrences of the PartialOverloading flag.

[francisco.lopes]

Refactoring

• Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

[francisco.lopes]

In D6880#109513, @francisco.lopes wrote:

Refactoring

• Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

Looking elsewhere and in docs, it seems suppressDiagnostics() is better to be used since diagnostics in lookups just for completion are not expected.

[francisco.lopes]

In D6880#109626, @francisco.lopes wrote:

In D6880#109513, @francisco.lopes wrote:

Refactoring

• Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

Looking elsewhere and in docs, it seems suppressDiagnostics() is better to be used since diagnostics in lookups just for completion are not expected.

I've tried a build without this line just to check whether I could get any effect, but no, it didn't make a difference as far as I can see, so... ¯\_(ツ)_/¯.

[francisco.lopes]

Minor cleanup.

[klimek]

This looks much simpler and easier to understand now - thanks for putting in the effort!

lib/Sema/SemaCodeComplete.cpp

3824–3828 ↗

(On Diff #18288)

http://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h
"""Prefer to use SmallVectorImpl<T> as a parameter type."""
That also removes the need for this to be templated at all iiuc.

3892–3895 ↗

(On Diff #18288)

I still don't understand why we need this instead of just calling getExplicitTemplateArgs when we need it...

3919–3920 ↗

(On Diff #18288)

Add "." in the end of a sentence. Also, thanks for adding those comments, they help me tremendously :)

3855–3857 ↗

(On Diff #18215)

Ah, I now see where this has gone :)

3859–3861 ↗

(On Diff #18215)

Just a general rule of thumb: if you copy something non-trivial from somewhere else (where you're not sure what exactly it does either), consider pulling out a function.

lib/Sema/SemaOverload.cpp

12490–12495

1. are we not in namespace clang yet??
2. This could still need a comment explaining exactly why we need the +1 - the next person coming along will wonder the same thing, so we can save them a lot of time by leaving a comprehensive explanation... :)

[francisco.lopes]

lib/Sema/SemaCodeComplete.cpp

3824–3828 ↗

(On Diff #18288)

Thanks, I missed this tip.

3892–3895 ↗

(On Diff #18288)

AddFunctionCandidates expects TemplateArgumentListInfo * which is not compatible with the return of getExplicitTemplateArgs or getOptionalExplicitTemplateArgs.

3919–3920 ↗

(On Diff #18288)

OK.

lib/Sema/SemaOverload.cpp

12490–12495

1. If you're just asking: no, I have declared it in namespace clang (since it's used in SemaTemplateDeduction.cpp) and must be defined inside it.
   
   If you're asking why it's in namespace clang and not elsewhere: it was the best place I could infer.

2. OK.

[francisco.lopes]

Refactoring.

[francisco.lopes]

Fix typo.

[klimek]

lib/Sema/SemaOverload.cpp

12490–12495

Comment is perfect, thanks!
I find the definition as clang:: strange; can we instead just put it into an anonymous namespace? (is it used anywhere else?)

[francisco.lopes]

lib/Sema/SemaOverload.cpp

12490–12495

By now, it's used in SemaTemplateDeduction.cpp, and SemaOverload.cpp which is where it was defined as it seems to best match the context for its definition. It may also be used elsewhere futurely in case there's a need to apply the same logic.

[francisco.lopes]

Very minor typo.

[francisco.lopes]

Adds some FIXME notes for things that still are not working.

[francisco.lopes]

Fix a unintended usage of canonical declaration.

[francisco.lopes]

Removes unneeded typedef (refactoring side-effect).

[francisco.lopes]

Fix issue with completing constructors for template specializations instantiated from a template
due to an explicit instantiation declaration request.

The two remaining known FIXME's should be addressed in another patch I believe. I know how
to solve the one about optional arguments which should make overload completion resemble
what's already implemented for functions in general completion.

[klimek]

include/clang/Sema/Sema.h

8775–8776 ↗

(On Diff #18416)

Ok, after thinking about it for a bit, I believe we should put this into Sema (and add a comment).

lib/Sema/SemaCodeComplete.cpp

3843–3844 ↗

(On Diff #18416)

I think the names are a bit off, we need a comment, and slightly modify the function (the Args array is not used for anything but the size).
How about:

Get the type of the Nth parameter from a given candidate set.
Given the overloads 'Candidates' for a function call matching all arguments up to
N, return the type of the Nth parameter if it is the same for all overload candidates.
Otherwise return a default-constructed QualType.
QualType getParamType(Sema &S, ArrayRef<ResultCandidate> Candidates, int N)

3893–3896 ↗

(On Diff #18416)

I now understand the need for copying, but not why you need an extra variable for the pointer instead of just passing &TemplateArgsBuffer below.

3942–3953 ↗

(On Diff #18416)

This seems to also be pretty much the same as in the function below; I'd pull out a function for that, too.

3961–3963 ↗

(On Diff #18416)

Space after 'if'; generally, use clang-format with -style=LLVM

3966 ↗

(On Diff #18416)

perhaps "highlighting optional parameters"?

[francisco.lopes]

Thanks for the detailed feedback Manuel.

include/clang/Sema/Sema.h

8775–8776 ↗

(On Diff #18416)

Ok.

lib/Sema/SemaCodeComplete.cpp

3843–3844 ↗

(On Diff #18416)

Ok, thanks for suggestion. I've took freedom to change the brief a bit since the commentary is long and with multiple sentences. Let me know whether it's good the way I've left it.

3893–3896 ↗

(On Diff #18416)

There's a convention in the codebase that a parameter of type TemplateArgumentListInfo *, for explicit template arguments, may be checked for nullptr by the callee. Also, this snippet follows a pattern used elsewhere in the codebase.

3942–3953 ↗

(On Diff #18416)

Ok.

3961–3963 ↗

(On Diff #18416)

Thanks, I'm sorry for that, I've fixed this if and the surrounding for. I'm following formatting by hand since using clang-format over the file changes a lot of unrelated stuff. Any tips to pass it over the file? Only option I see is using clang-format over the particular lines I'm editing.

3966 ↗

(On Diff #18416)

Ok.

[francisco.lopes]

Refactoring, round 2

• Followed all points by Manuel Klimek.

[francisco.lopes]

Typo. Not used to being strict to period in docs...

[klimek]

Ok, looks good now. Thanks! This is a really nice improvement.

lib/Sema/SemaCodeComplete.cpp

3893–3896 ↗

(On Diff #18416)

/me headdesks; (the reason is that we only have it if UME->hasExplicitTemplateArgs(), I completely missed that).

[francisco.lopes]

Landed in Clang, SVN revision 226670.

lib/Sema/SemaOverload.cpp

EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
   return nullptr;
 }
 
 /// \brief Add all of the function declarations in the given function set to
 /// the overload candidate set.
 void Sema::AddFunctionCandidates(const UnresolvedSetImpl &Fns,
                                  ArrayRef<Expr *> Args,
                                  OverloadCandidateSet& CandidateSet,
                                TemplateArgumentListInfo *ExplicitTemplateArgs,
                                  bool SuppressUserConversions,
                                  bool PartialOverloading) {
   for (UnresolvedSetIterator F = Fns.begin(), E = Fns.end(); F != E; ++F) {
     NamedDecl *D = F.getDecl()->getUnderlyingDecl();
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
       if (isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->isStatic() &&
           !isa<CXXConstructorDecl>(FD))
         AddMethodCandidate(cast<CXXMethodDecl>(FD), F.getPair(),
                            cast<CXXMethodDecl>(FD)->getParent(),
                            Args[0]->getType(), Args[0]->Classify(Context),
                            Args.slice(1), CandidateSet,
                            SuppressUserConversions, PartialOverloading);
       else
         AddOverloadCandidate(FD, F.getPair(), Args, CandidateSet,
                              SuppressUserConversions, PartialOverloading);
     } else {
Sema::AddMethodCandidate(CXXMethodDecl *Method, DeclAccessPair FoundDecl,
   Candidate.IsSurrogate = false;
   Candidate.IgnoreObjectArgument = false;
   Candidate.ExplicitCallArguments = Args.size();
 
   unsigned NumParams = Proto->getNumParams();
 
   // (C++ 13.3.2p2): A candidate function having fewer than m
   // parameters is viable only if it has an ellipsis in its parameter
   // list (8.3.5).
   if ((Args.size() + (PartialOverloading && Args.size())) > NumParams &&
       !Proto->isVariadic()) {
     Candidate.Viable = false;
     Candidate.FailureKind = ovl_fail_too_many_arguments;
     return;
   }
 
   // (C++ 13.3.2p2): A candidate function having more than m parameters
   // is viable only if the (m+1)st parameter has a default argument
   // (8.3.6). For the purposes of overload resolution, the
   // parameter list is truncated on the right, so that there are
if (TemplateDeductionResult Result
     for (unsigned I = 0; I < Args.size() && ShouldAdd; ++I) {
       // (C++ 13.3.2p2): For the purposes of overload resolution, any
       // argument for which there is no corresponding parameter is
       // considered to ""match the ellipsis" (C+ 13.3.3.1.3).
       Candidate.Conversions[I].setEllipsis();
     }
     Candidate.FoundDecl = FoundDecl;
     Candidate.Function = FunctionTemplate->getTemplatedDecl();
-    Candidate.Viable = PartialOverloading && Result == TDK_TooManyArguments;
+    Candidate.Viable = ShouldAdd;
     Candidate.FailureKind = ovl_fail_bad_deduction;
     Candidate.IsSurrogate = false;
     Candidate.IgnoreObjectArgument = false;
     Candidate.ExplicitCallArguments = Args.size();
     Candidate.DeductionFailure = MakeDeductionFailureInfo(Context, Result,
                                                           Info);
     return;
   }
Expr *Sema::FixOverloadedFunctionReference(Expr *E, DeclAccessPair Found,
 
   llvm_unreachable("Invalid reference to overloaded function");
 }
 
 ExprResult Sema::FixOverloadedFunctionReference(ExprResult E,
                                                 DeclAccessPair Found,
                                                 FunctionDecl *Fn) {
   return FixOverloadedFunctionReference(E.get(), Found, Fn);
 }