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

[Manuel Klimek]

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


================
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 || 
----------------
francisco.lopes wrote:
> klimek wrote:
> > Why is it ok to remove the DeclRefExpr case? Where is this handled in the new code?
> 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.
Ah, I now see where this has gone :)

================
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(),
----------------
francisco.lopes wrote:
> klimek wrote:
> > This needs a comment explaining why we do that; why is it a simple lvalue if UnresExpr->isArrow()? Why not use Classify unconditionally?
> 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.
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.

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3824-3828
@@ +3823,7 @@
+
+template <unsigned N>
+void mergeCandidatesWithResults(SmallVector<ResultCandidate, N> &Results,
+                                OverloadCandidateSet &CandidateSet,
+                                SourceLocation Loc,
+                                Sema &S) {
+  if (!CandidateSet.empty()) {
----------------
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.

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3892-3895
@@ +3891,6 @@
+    TemplateArgumentListInfo TemplateArgsBuffer, *TemplateArgs = nullptr;
+    if (UME->hasExplicitTemplateArgs()) {
+      UME->copyTemplateArgumentsInto(TemplateArgsBuffer);
+      TemplateArgs = &TemplateArgsBuffer;
+    }
+    SmallVector<Expr*, 12> ArgExprs(1, UME->getBase());
----------------
I still don't understand why we need this instead of just calling getExplicitTemplateArgs when we need it...

================
Comment at: lib/Sema/SemaCodeComplete.cpp:3919-3920
@@ +3918,4 @@
+  } else {
+    // Lastly we check, as a possibly resolved expression, whether it can be
+    // converted to a function
+    FunctionDecl *FD = nullptr;
----------------
Add "." in the end of a sentence. Also, thanks for adding those comments, they help me tremendously :)

================
Comment at: lib/Sema/SemaOverload.cpp:12477-12482
@@ +12476,7 @@
+
+bool clang::TooManyArguments(size_t NumParams, size_t NumArgs,
+                             bool PartialOverloading) {
+  if (NumArgs > 0 && PartialOverloading)
+    return NumArgs + 1 > NumParams;
+  return NumArgs > NumParams;
+}
----------------
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... :)

http://reviews.llvm.org/D6880

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