This is an archive of the discontinued LLVM Phabricator instance.

Implementation for completion in call context for C++
ClosedPublic

Authored by francisco.lopes on Jan 8 2015, 10:19 AM.

Details

Summary

Improved completion in call context to work with:

  • functions
  • member functions
  • constructors
  • new expressions
  • function call expressions
  • template variants of the previous

Diff Detail

Event Timeline

francisco.lopes retitled this revision from to Implementation for completion in call context for C++.
francisco.lopes updated this object.
francisco.lopes edited the test plan for this revision. (Show Details)
francisco.lopes added a reviewer: klimek.
francisco.lopes added a subscriber: Unknown Object (MLST).

Basic functor parameter completion was added.

It's currently working for:

struct S { void operator()(int){} };
int main() {
    S s;
    s(>|<);
}

but not for:

struct S { void operator()(int){} };
int main() {
    S{}(>|<);
}

+Richard, Doug, who needs to sign off on changes to code completion behavior?

I expect to do some refactoring to reduce code duplication if possible in the following days, and maybe get the extended support for constructors, etc. The current state can be merged now without problems, since the current behavior will be the same after I do the remaining changes.

I can do the merge myself, in case I get a review.

francisco.lopes edited edge metadata.

Good amount of refactoring. Now providing extend support for function call operator.

+chandler, since nobody else gave any pointers at who needs to approve this

Support for constructors and new expressions.

@klimek Hi, I'm providing differential diffs, not sure whether they should have been accumulative. I think it's in good enough shape now, I don't expect to tweak it for a while.

As I said, we'll first need to find somebody who knows the trade-offs of that part of the code base to be able to sign off on the high level functionality change - did you already send an email with that to cfe-dev by the way? That might be the better venue for a high level discussion of the feature...

As per conversation at cfe-dev, I've agreed it's best for now to have the original ordinary name code-completion behavior preserved while providing the new set of overload candidates completions.

I had to create an additional CXCursorKind named CXCursor_OverloadCandidate, so that there's any way to discern overload candidates results.

Futurely there may be an API with specific purpose of querying for overload candidates solely, I won't implement it for now.

The tests that were deprecated before were added back.

francisco.lopes updated this object.
francisco.lopes updated this object.
francisco.lopes updated this object.

If I'm not mistaken I should have been using Arcanist erroneously by submitting relative diffs.

This patch is being produced solely to provide an unified diff.

General high level comment: you're using C++11 extensively, have you made sure you're only using features MSVC 2012 supports (as per the style guide: http://llvm.org/docs/CodingStandards.html)?

Thanks for the feedback Manuel,

I didn't try it in MSVC 2012, I hope it's not a hassle to compile clang in
Windows,
I'm downloading the express edition so I can try it.

I believe the sole critical piece regarding the style guide is the
declaration of the
ParseExpressionList function. Although I think it's good to have the
default argument
initialized that way, which is simple and short, it may not be in
accordance with the
"Do not use Braced Initializer Lists to Call a Constructor" section of the
style guide.

As I'm unsure about this specific usage, I'll left it as is until proper
review.

2015-01-13 17:12 GMT-02:00 Manuel Klimek <klimek@google.com>:

General high level comment: you're using C++11 extensively, have you made
sure you're only using features MSVC 2012 supports (as per the style guide:
http://llvm.org/docs/CodingStandards.html)?

http://reviews.llvm.org/D6880

EMAIL PREFERENCES

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

Thanks for the feedback Manuel,

I didn't try it in MSVC 2012, I hope it's not a hassle to compile clang in
Windows,
I'm downloading the express edition so I can try it.

I believe the sole critical piece regarding the style guide is the
declaration of the
ParseExpressionList function. Although I think it's good to have the
default argument
initialized that way, which is simple and short, it may not be in
accordance with the
"Do not use Braced Initializer Lists to Call a Constructor" section of the
style guide.

As I'm unsure about this specific usage, I'll left it as is until proper
review.

2015-01-13 17:12 GMT-02:00 Manuel Klimek <klimek@google.com>:

General high level comment: you're using C++11 extensively, have you made
sure you're only using features MSVC 2012 supports (as per the style guide:
http://llvm.org/docs/CodingStandards.html)?

http://reviews.llvm.org/D6880

EMAIL PREFERENCES

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

Yeah, MSVC 2012 doesn't like copy-list-initialization either. Fixed, thanks again for the feedback.

klimek added inline comments.Jan 14 2015, 12:51 AM
lib/Sema/SemaOverload.cpp
5854

Indent.

5860–5861

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?

5966–5967

This is a little too clever for me :)
a) comment on why we need to +1 when PartialOverloading
b) pull it out of the if into a variable?

lib/Sema/SemaOverload.cpp
5860–5861

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.

5966–5967

For me too! but I've just copied the behavior from some original place. Changing all references of such snippet would mean to enter refactoring of the original code. While applying these changes I paused for a moment to analyze what the **** this was doing. I think I got it for a moment and didn't care anymore and just replicated where it was necessary.

Do you think this is time for refactoring the changeset and the remaining references of this
code?

klimek added inline comments.Jan 14 2015, 6:24 AM
lib/Sema/SemaOverload.cpp
5860–5861

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.

5966–5967

You're right, now is not the time for a refactoring.

lib/Sema/SemaOverload.cpp
5860–5861

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.

klimek added inline comments.Jan 14 2015, 7:01 AM
lib/Sema/SemaOverload.cpp
5860–5861

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.

Changes suggested by @klimek were applied.

The latest version seems to be exactly the same as the previous version :)

The latest version seems to be exactly the same as the previous version :)

Phabricator's Differential tool doesn't display whitespace changes (I've followed the small indentation suggestion) and also it's not possible to show differences through the commits, only through the patches that are being sent. I've rebased and added a new commit, but the changeset as a whole only changes regarding the indentation. You may check what happened at https://github.com/oblitum/clang/tree/improved_argument_hints. I finding Phabricator awkward to use for these kind of modifications.

The latest version seems to be exactly the same as the previous version :)

Phabricator's Differential tool doesn't display whitespace changes (I've followed the small indentation suggestion) and also it's not possible to show differences through the commits, only through the patches that are being sent. I've rebased and added a new commit, but the changeset as a whole only changes regarding the indentation. You may check what happened at https://github.com/oblitum/clang/tree/improved_argument_hints. I finding Phabricator awkward to use for these kind of modifications.

Notice that in Differential the new commits are being listed, it's just not possible to see what they actually do...

Yes, phabricator doesn't support using a single branch for multiple diffs well - the best way to use it is to create feature branches that each have a single feature...

Removing the fix that avoids AddFunctionCandidates from calling AddMethodCandidate and AddMethodTemplateCandidate (which it shoudn't) out to a different patch to be applied separately.

I'll commit this fix separately after this patch gets merged upstream.

Thx for your patience; just to be clear: I think this is a great patch, but I'll need to get it down into chunks I can bite off from as reviewer :)

lib/Sema/SemaOverload.cpp
5966–5968

Ok, sorry for the many questions, but I'm new to this part of the code myself :)
This also changes the overload resolution, right? Is this an integral part of the patch, or could this also be split out? If it cannot be split out, can you explain why we now need this and which test cases apply to that?

lib/Sema/SemaOverload.cpp
5966–5968
  • Yes, this kind of changes that involve the PartialOverloading flag should change the behavior for overloading resolution solely when it's true.
  • Yes, it's an integral part of the patch. As can be seen the original codebase already have this kind of check where it's already supporting "partial" overloading. It can't be splitted out.

As can be checked in SemaOverload.cpp:

/// AddOverloadCandidate - Adds the given function to the set of
/// candidate functions, using the given function call arguments.  If
/// @p SuppressUserConversions, then don't allow user-defined
/// conversions via constructors or conversion operators.
///
/// \param PartialOverloading true if we are performing "partial" overloading
/// based on an incomplete set of function arguments. This feature is used by
/// code completion.

PartialOverloading is used in code completion.

The most basic situation to make sense of this code is when code completion is requested in calling context, just after a comma for an argument. Let's say the prototype of the function being called has just one parameter. So in this test NumParams would be 1. As code completion was triggered just after the comma to insert a second argument that was still not inserted, Args.size() is still 1. So Args.size() > NumParams is false but the test must still succeed to set Candidate.FailureKind = ovl_fail_too_few_arguments since the extra comma means a second argument follows.

Notice that even though the code is being correct at this moment for failing with ovl_fail_too_few_arguments, this relates with another part of the code where I do add all candidates that failed with this reason to the result set of viable candidates.

The reason I'm adding the candidates that fail because of "too many arguments" to the result set is because the user will be able to obtain the overloads of that function so it can be used for displaying the expected prototypes, and also it will be able to verify when the overloads are not matching because they won't be providing a "current argument", which all successful overloads with more that one argument will be providing.

Anyway, this last part is still open for discussion although I think a change regarding this coming afterwards won't harm either.

lib/Sema/SemaOverload.cpp
5966–5968

Correction: which all successful overloads with one parameter or more will be providing.

Thx for your patience; just to be clear: I think this is a great patch, but I'll need to get it down into chunks I can bite off from as reviewer :)

I'm unsure whether you're just generally commenting regarding the incoming questions about the patch, or whether you're requesting to cut the patch somehow for it to be commited in separate chunks.

Handles force added overloads in similar fashion

The way candidates that failed in overload resolution (for reason of too
many arguments) were being force added wasn't symmetric. This makes this
forced inclusion similar in all cases, which ends up having side-effects
in how such candidates gets sorted. The sorting now looks improved.

I'm really unsure about having this feature. It seems useful at the same time
as it doesn't seem righteous from the point of view of being strict and not giving
any results for extra commas.

This commit removes the feature I've mentioned before, after having improving it =D

This commit removes the feature I've mentioned before, after having improving it =D

I've also rebased the changeset over upstream since it has been some time already.

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

lib/Sema/SemaCodeComplete.cpp
3855–3857

Why is it ok to remove the DeclRefExpr case? Where is this handled in the new code?

3890–3896

Generally, try to keep the scope of variables as short as possible. Just declare stuff in the loop if it's only used there.

3897–3899

This needs a comment explaining why we do that; why is it a simple lvalue if UnresExpr->isArrow()? Why not use Classify unconditionally?

3900–3901

I thought be now we should have a way to use a range based for loop here?
for (auto Decl : UnresExpr->decls()) {

...

}

3915

You can declare Method in the if:
} else if (CXXMethodDecl *Method = dyn_cast...

3918

I'd just inline UnresExpr->hasExplicitTemplateArgs() here.

3927

Any reason to not just use &UnresExpr->getExplicitTemplateArgs()?

3934–3935

This needs a comment as to what exactly we're trying to address here...

3940

Is the suppressDiagnostics() really needed (I'm not familiar with this, so I just want to make sure :)

3950–3954

A short description on what we're going for here would also help tremendously :)

3986–4017

This looks copied. Pull out a function.

lib/Sema/SemaOverload.cpp
5965–5968

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).

lib/Sema/SemaCodeComplete.cpp
3855–3857

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.

3890–3896

OK.

3897–3899

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.

3900–3901

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.

3915

Thanks, this will not apply anymore.

3918

Thanks, this will not apply anymore.

3927

Thanks, this will not apply anymore.

3934–3935

OK.

3940

I'm not sure, will check after the first batch of refactorings.

3950–3954

OK.

3986–4017

OK.

lib/Sema/SemaOverload.cpp
5965–5968

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.

Refactoring

  • Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

Refactoring

  • Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

Looking elsewhere and in docs, it seems suppressDiagnostics() is better to be used since diagnostics in lookups just for completion are not expected.

Refactoring

  • Followed all points by Manuel Klimek except for the one targeting suppressDiagnostics().

Looking elsewhere and in docs, it seems suppressDiagnostics() is better to be used since diagnostics in lookups just for completion are not expected.

I've tried a build without this line just to check whether I could get any effect, but no, it didn't make a difference as far as I can see, so... ¯\_(ツ)_/¯.

Minor cleanup.

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

lib/Sema/SemaCodeComplete.cpp
3824–3828

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.

3855–3857

Ah, I now see where this has gone :)

3891–3894

I still don't understand why we need this instead of just calling getExplicitTemplateArgs when we need it...

3897–3899

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.

3918–3919

Add "." in the end of a sentence. Also, thanks for adding those comments, they help me tremendously :)

lib/Sema/SemaOverload.cpp
12477–12482
  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... :)
lib/Sema/SemaCodeComplete.cpp
3824–3828

Thanks, I missed this tip.

3891–3894

AddFunctionCandidates expects TemplateArgumentListInfo * which is not compatible with the return of getExplicitTemplateArgs or getOptionalExplicitTemplateArgs.

3918–3919

OK.

lib/Sema/SemaOverload.cpp
12477–12482
  1. If you're just asking: no, I have declared it in namespace clang (since it's used in SemaTemplateDeduction.cpp) and must be defined inside it.

    If you're asking why it's in namespace clang and not elsewhere: it was the best place I could infer.
  1. OK.
klimek added inline comments.Jan 16 2015, 5:11 AM
lib/Sema/SemaOverload.cpp
12477–12482

Comment is perfect, thanks!
I find the definition as clang:: strange; can we instead just put it into an anonymous namespace? (is it used anywhere else?)

lib/Sema/SemaOverload.cpp
12477–12482

By now, it's used in SemaTemplateDeduction.cpp, and SemaOverload.cpp which is where it was defined as it seems to best match the context for its definition. It may also be used elsewhere futurely in case there's a need to apply the same logic.

Very minor typo.

Adds some FIXME notes for things that still are not working.

Fix a unintended usage of canonical declaration.

Removes unneeded typedef (refactoring side-effect).

Fix issue with completing constructors for template specializations instantiated from a template
due to an explicit instantiation declaration request.

The two remaining known FIXME's should be addressed in another patch I believe. I know how
to solve the one about optional arguments which should make overload completion resemble
what's already implemented for functions in general completion.

klimek added inline comments.Jan 20 2015, 4:39 AM
include/clang/Sema/Sema.h
8775–8776

Ok, after thinking about it for a bit, I believe we should put this into Sema (and add a comment).

lib/Sema/SemaCodeComplete.cpp
3843–3844

I think the names are a bit off, we need a comment, and slightly modify the function (the Args array is not used for anything but the size).
How about:

Get the type of the Nth parameter from a given candidate set.
Given the overloads 'Candidates' for a function call matching all arguments up to
N, return the type of the Nth parameter if it is the same for all overload candidates.
Otherwise return a default-constructed QualType.
QualType getParamType(Sema &S, ArrayRef<ResultCandidate> Candidates, int N)

3890–3893

I now understand the need for copying, but not why you need an extra variable for the pointer instead of just passing &TemplateArgsBuffer below.

3939–3979

This seems to also be pretty much the same as in the function below; I'd pull out a function for that, too.

3958–3960

Space after 'if'; generally, use clang-format with -style=LLVM

3963

perhaps "highlighting optional parameters"?

Thanks for the detailed feedback Manuel.

include/clang/Sema/Sema.h
8775–8776

Ok.

lib/Sema/SemaCodeComplete.cpp
3843–3844

Ok, thanks for suggestion. I've took freedom to change the brief a bit since the commentary is long and with multiple sentences. Let me know whether it's good the way I've left it.

3890–3893

There's a convention in the codebase that a parameter of type TemplateArgumentListInfo *, for explicit template arguments, may be checked for nullptr by the callee. Also, this snippet follows a pattern used elsewhere in the codebase.

3939–3979

Ok.

3958–3960

Thanks, I'm sorry for that, I've fixed this if and the surrounding for. I'm following formatting by hand since using clang-format over the file changes a lot of unrelated stuff. Any tips to pass it over the file? Only option I see is using clang-format over the particular lines I'm editing.

3963

Ok.

Refactoring, round 2

  • Followed all points by Manuel Klimek.

Typo. Not used to being strict to period in docs...

klimek accepted this revision.Jan 21 2015, 5:13 AM
klimek edited edge metadata.

Ok, looks good now. Thanks! This is a really nice improvement.

lib/Sema/SemaCodeComplete.cpp
3890–3893

/me headdesks; (the reason is that we only have it if UME->hasExplicitTemplateArgs(), I completely missed that).

This revision is now accepted and ready to land.Jan 21 2015, 5:13 AM

Landed in Clang, SVN revision 226670.