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/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
