[PATCH] Implementation for completion in call context for C++
Francisco Lopes da Silva
francisco.mailing.lists at oblita.com
Thu Jan 15 15:07:38 PST 2015
================
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
----------------
klimek wrote:
> Generally, try to keep the scope of variables as short as possible. Just declare stuff in the loop if it's only used there.
OK.
================
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 ||
----------------
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.
================
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(),
----------------
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.
================
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) {
+
----------------
klimek wrote:
> I thought be now we should have a way to use a range based for loop here?
> for (auto Decl : UnresExpr->decls()) {
> ...
> }
iterators have `getPair()`, which is used inside the loop, so the range-for may not be an option. This made me notice `I` is of type `UnresolvedSetIterator`, which made me realize this section could be entirely refactored based on a snippet of code I've produced myself.
================
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
----------------
klimek wrote:
> You can declare Method in the if:
> } else if (CXXMethodDecl *Method = dyn_cast...
Thanks, this will not apply anymore.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:3880
@@ +3879,3 @@
+ // non-template member function.
+ if (TemplateArgs)
+ continue;
----------------
klimek wrote:
> I'd just inline UnresExpr->hasExplicitTemplateArgs() here.
Thanks, this will not apply anymore.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:3889
@@ +3888,3 @@
+ AddMethodTemplateCandidate(cast<FunctionTemplateDecl>(Func),
+ I.getPair(), ActingDC, TemplateArgs,
+ ObjectType, ObjectClassification,
----------------
klimek wrote:
> Any reason to not just use &UnresExpr->getExplicitTemplateArgs()?
Thanks, this will not apply anymore.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:3896-3897
@@ +3895,4 @@
+ }
+ } else if (auto DC = NakedFn->getType()->getCanonicalTypeInternal()
+ ->getAsCXXRecordDecl()) {
+ DeclarationName OpName = Context.DeclarationNames
----------------
klimek wrote:
> This needs a comment as to what exactly we're trying to address here...
OK.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:3902
@@ +3901,3 @@
+ LookupQualifiedName(R, DC);
+ R.suppressDiagnostics();
+ SmallVector<Expr*, 12> ArgExprs(1, NakedFn);
----------------
klimek wrote:
> Is the suppressDiagnostics() really needed (I'm not familiar with this, so I just want to make sure :)
I'm not sure, will check after the first batch of refactorings.
================
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) {
----------------
klimek wrote:
> A short description on what we're going for here would also help tremendously :)
OK.
================
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;
+ }
+ }
}
}
----------------
klimek wrote:
> This looks copied. Pull out a function.
OK.
================
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;
----------------
klimek wrote:
> 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).
It's necessary, for example, using solely `Args.size() > NumParams` in the former original location leads to `test/CodeCompletion/call.cpp` to fail with the following:
```
<stdin>:44:19: error: CHECK-CC2-NOT: string occurred!
OVERLOAD: [#void#]f(N::Y y, int ZZ)
^
/opt/src/llvm/tools/clang/test/CodeCompletion/call.cpp:26:20: note: CHECK-CC2-NOT: pattern specified here
// CHECK-CC2-NOT: f(N::Y y, int ZZ)
```
The reason for it to fail follows exacly from my explanation. It's interesting that there isn't much usage of negative checks in the tests, so I coudn't think of using them myself.
The only option is to refactor the test, it was not used elsewhere because support for completion in call context was minimal, as also the occurrences of the `PartialOverloading` flag.
http://reviews.llvm.org/D6880
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list