[PATCH] Implementation for completion in call context for C++

Manuel Klimek klimek at google.com
Wed Jan 14 07:01:07 PST 2015


================
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:
> > 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.
> 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.
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.

http://reviews.llvm.org/D6880

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






More information about the cfe-commits mailing list