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

[Manuel Klimek]

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


================
Comment at: lib/Sema/SemaCodeComplete.cpp:3852-3858
@@ +3851,9 @@
+  else if (auto UnresExpr = dyn_cast<UnresolvedMemberExpr>(NakedFn)) {
+    CXXMethodDecl *Method = nullptr;
+    QualType ObjectType = UnresExpr->getBaseType();
+    TemplateArgumentListInfo TemplateArgsBuffer, *TemplateArgs = nullptr;
+    if (UnresExpr->hasExplicitTemplateArgs()) {
+      UnresExpr->copyTemplateArgumentsInto(TemplateArgsBuffer);
+      TemplateArgs = &TemplateArgsBuffer;
+    }
+    Expr::Classification ObjectClassification
----------------
Generally, try to keep the scope of variables as short as possible. Just declare stuff in the loop if it's only used there.

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3855-3857
@@ -3854,5 +3918,1 @@
-                                /*PartialOverloading=*/ true);
-  else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(NakedFn)) {
-    FunctionDecl *FDecl = dyn_cast<FunctionDecl>(DRE->getDecl());
-    if (FDecl) {
       if (!getLangOpts().CPlusPlus || 
----------------
Why is it ok to remove the DeclRefExpr case? Where is this handled in the new code?

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3859-3861
@@ +3858,5 @@
+    }
+    Expr::Classification ObjectClassification
+      = UnresExpr->isArrow()? Expr::Classification::makeSimpleLValue()
+                            : UnresExpr->getBase()->Classify(Context);
+    for (auto I = UnresExpr->decls_begin(),
----------------
This needs a comment explaining why we do that; why is it a simple lvalue if UnresExpr->isArrow()? Why not use Classify unconditionally?

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3862-3863
@@ +3861,4 @@
+                            : UnresExpr->getBase()->Classify(Context);
+    for (auto I = UnresExpr->decls_begin(),
+           E = UnresExpr->decls_end(); I != E; ++I) {
+
----------------
I thought be now we should have a way to use a range based for loop here?
for (auto Decl : UnresExpr->decls()) {
  ...
}

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3877
@@ +3876,3 @@
+                             /*PartialOverloading=*/true);
+      } else if ((Method = dyn_cast<CXXMethodDecl>(Func))) {
+        // If explicit template arguments were provided, we can't call a
----------------
You can declare Method in the if:
} else if (CXXMethodDecl *Method = dyn_cast...

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3880
@@ +3879,3 @@
+        // non-template member function.
+        if (TemplateArgs)
+          continue;
----------------
I'd just inline UnresExpr->hasExplicitTemplateArgs() here.

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3889
@@ +3888,3 @@
+        AddMethodTemplateCandidate(cast<FunctionTemplateDecl>(Func),
+                                   I.getPair(), ActingDC, TemplateArgs,
+                                   ObjectType,  ObjectClassification,
----------------
Any reason to not just use &UnresExpr->getExplicitTemplateArgs()?

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3896-3897
@@ +3895,4 @@
+    }
+  } else if (auto DC = NakedFn->getType()->getCanonicalTypeInternal()
+                       ->getAsCXXRecordDecl()) {
+    DeclarationName OpName = Context.DeclarationNames
----------------
This needs a comment as to what exactly we're trying to address here...

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3902
@@ +3901,3 @@
+    LookupQualifiedName(R, DC);
+    R.suppressDiagnostics();
+    SmallVector<Expr*, 12> ArgExprs(1, NakedFn);
----------------
Is the suppressDiagnostics() really needed (I'm not familiar with this, so I just want to make sure :)

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3912-3916
@@ +3911,7 @@
+  } else {
+    FunctionDecl *FD = nullptr;
+    if (auto MCE = dyn_cast<MemberExpr>(NakedFn))
+      FD = dyn_cast<FunctionDecl>(MCE->getMemberDecl());
+    else if (auto DRE = dyn_cast<DeclRefExpr>(NakedFn))
+      FD = dyn_cast<FunctionDecl>(DRE->getDecl());
+    if (FD) {
----------------
A short description on what we're going for here would also help tremendously :)

================
Comment at: lib/Sema/SemaCodeComplete.cpp:4006-4037
@@ -3919,2 +4005,34 @@
+
+  if (!CandidateSet.empty()) {
+    // Sort the overload candidate set by placing the best overloads first.
+    std::stable_sort(
+        CandidateSet.begin(), CandidateSet.end(),
+        [&](const OverloadCandidate &X, const OverloadCandidate &Y) {
+          return isBetterOverloadCandidate(*this, X, Y, Loc);
+        });
+
+    // Add the remaining viable overload candidates as code-completion results.
+    for (OverloadCandidateSet::iterator Cand = CandidateSet.begin(),
+                                     CandEnd = CandidateSet.end();
+         Cand != CandEnd; ++Cand) {
+      if (Cand->Viable)
+        Results.push_back(ResultCandidate(Cand->Function));
+    }
+
+    // From the viable candidates, try to determine the type of this parameter.
+    for (unsigned I = 0, N = Results.size(); I != N; ++I) {
+      if (const FunctionType *FType = Results[I].getFunctionType())
+        if (const FunctionProtoType *Proto = dyn_cast<FunctionProtoType>(FType))
+          if (Args.size() < Proto->getNumParams()) {
+            if (ParamType.isNull())
+              ParamType = Proto->getParamType(Args.size());
+            else if (!Context.hasSameUnqualifiedType(
+                          ParamType.getNonReferenceType(),
+                          Proto->getParamType(Args.size())
+                              .getNonReferenceType())) {
+              ParamType = QualType();
+              break;
+            }
+          }
     }
   }
----------------
This looks copied. Pull out a function.

================
Comment at: lib/Sema/SemaOverload.cpp:5965-5967
@@ -5959,4 +5964,5 @@
   // parameters is viable only if it has an ellipsis in its parameter
   // list (8.3.5).
-  if (Args.size() > NumParams && !Proto->isVariadic()) {
+  if ((Args.size() + (PartialOverloading && Args.size())) > NumParams &&
+      !Proto->isVariadic()) {
     Candidate.Viable = false;
----------------
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).

http://reviews.llvm.org/D6880

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