================
Comment at: lib/Sema/SemaOverload.cpp:5860-5861
@@ -5857,3 +5859,4 @@
if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
- if (isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->isStatic())
+ if (isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->isStatic() &&
+ !isa<CXXConstructorDecl>(FD))
AddMethodCandidate(cast<CXXMethodDecl>(FD), F.getPair(),
----------------
francisco.lopes wrote:
> klimek wrote:
> > 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?
> 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.
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.
================
Comment at: lib/Sema/SemaOverload.cpp:5968
@@ -5960,2 +5967,3 @@
// list (8.3.5).
- if (Args.size() > NumParams && !Proto->isVariadic()) {
+ if ((Args.size() + (PartialOverloading && Args.size())) > NumParams &&
+ !Proto->isVariadic()) {
----------------
francisco.lopes wrote:
> klimek wrote:
> > 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?
> 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?
You're right, now is not the time for a refactoring.
http://reviews.llvm.org/D6880
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
