Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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?

3918–3924

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

3925–3927

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

3928–3929

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

...

}

3943

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

3946

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

3955

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

3962–3963

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

3968

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

3978–3982

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

4008–4039

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.

3918–3924

OK.

3925–3927

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.

3928–3929

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.

3943

Thanks, this will not apply anymore.

3946

Thanks, this will not apply anymore.

3955

Thanks, this will not apply anymore.

3962–3963

OK.

3968

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

3978–3982

OK.

4008–4039

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

3919–3922

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

3925–3927

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.

3946–3947

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.

3919–3922

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

3946–3947

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
8785–8786

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)

3918–3921

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

3963–3997

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

3980–3982

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

3985

perhaps "highlighting optional parameters"?

Thanks for the detailed feedback Manuel.

include/clang/Sema/Sema.h
8785–8786

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.

3918–3921

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.

3963–3997

Ok.

3980–3982

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.

3985

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
3918–3921

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