<div dir="ltr"><div><div><div>Thanks for the input Milian, you did made me think it further, since I'm not used to<br></div>authoring tools that provide that kind of simultaneous results. I think I'll maintain the<br>current official behavior then, even though I believe this may turn the interface slower<br>for parameter completions that don't look for the extra information about possible fitting<br></div>suggestions. These additional cases require extra AST trips and returns too<br>much information, so, performance wise, maybe a future split in the API about this may<br>be worth looking at.<br><br></div><div>Notice that for disambiguation between the general cases and the call signature<br>suggestions one can check whether a given completion provides a "current argument"<br>token, so it's a call signature that fits, or check whether its cursor kind is CXCursor_NotImplemented,<br></div><div>which it still is. I think I'll change that so that these particular completion cases I'm<br></div><div>providing support for can be clearly distinguished from the rest.<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">If I'm not mistaking your video, your patch removes the list of results and<br>
replaces it only by a list of matching types? This would be an unfortunate<br>
regression, imo.<br></blockquote><br></div><div>What's is being provided as result in truth, is at the top-left of the video when I set completopt += preview,<br>which means the entire signatures that fits. From that signature I'm picking, <i>in the Vim plugin</i>, the current<br>argument to provide suggestions in the popup.<br></div><div>For more detail you can check the tests that are provided with the patches.<br><br></div><div>Despite this, I'll work to also provide the general fitting cases as explained before.<br><br>Regards,<br></div><div>Francisco Lopes<br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">2015-01-12 14:10 GMT-02:00 Milian Wolff <span dir="ltr"><<a href="mailto:mail@milianw.de" target="_blank">mail@milianw.de</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Monday 05 January 2015 19:15:48 Francisco Lopes wrote:<br>
> Hi,<br>
><br>
> I've improved completion in call context to work with C++'s templates,<br>
> member functions, et al.<br>
><br>
> Notice: an important aspect of this implementation is that I've took freedom<br>
> to change the previous behavior for completion in call context at all.<br>
><br>
> As can be seen, in lib/Sema/SemaCodeComplete.cpp this comment:<br>
><br>
>   // When we're code-completing for a call, we fall back to ordinary<br>
>   // name code-completion whenever we can't produce specific<br>
>   // results. We may want to revisit this strategy in the future,<br>
>   // e.g., by merging the two kinds of results.<br>
><br>
> was changed to this:<br>
><br>
>   // When we're code-completing for a call, we DON'T fall back to ordinary<br>
>   // name code-completion whenever we can't produce specific<br>
>   // results. This will do "parameter completion" solely.<br>
>   // I think "parameter completion" is not a good classification, since this<br>
> // is useful solely for providing hints and not code-completing.<br>
><br>
> The rationale behind this change is:<br>
>   - The former comment shows that completion in call context is still not<br>
>     in a definite state, open to discussion.<br>
>   - I firmly believe this new behavior is for good. In call context, 99% of<br>
>     the time, code authoring tools will be interested in information about<br>
> the<br>
>     prototype and arguments solely, not in all possible kinds of expressions<br>
> that can fit as an argument. This behavior turns the interface slower than<br>
>     necessary to have almost no benefit.<br>
>     So I believe that completion in call context just after '(' and ','<br>
> should<br>
>     work similarly as for member access: only provide what's of interest,<br>
> which<br>
>     parametric information (function prototypes, current argument, parameter<br>
> types, etc.).<br>
><br>
> Despite this, I should be able revert to the former behavior in the change<br>
> set<br>
> without much trouble, in case there's no agreement on this, while still<br>
> providing the new set of completions.<br>
><br>
> The following is a video that covers the new behavior, so that's easy to see<br>
> what got changed. It's based on the test set that's part of the patch:<br>
><br>
>   <a href="https://vimeo.com/115990512" target="_blank">https://vimeo.com/115990512</a><br>
<br>
</div></div>Hey Francisco.<br>
<br>
I can't review the code, just wanted to comment on the functionality as I<br>
observe it from the video above. I think that the information about the call<br>
signatures during code completion is extremely useful. But it should not be<br>
put into the list of code completion results. Rather, I think it is additional<br>
information that should be distinguished from the results. These results<br>
should be a list of everything accessible at the call site, sorted by their<br>
relevance and excluding everything that can be ruled out.<br>
<br>
In KDevelop e.g. this looks like this:<br>
<br>
<a href="http://imgur.com/odCbYrU" target="_blank">http://imgur.com/odCbYrU</a><br>
<a href="http://imgur.com/xxN2isv" target="_blank">http://imgur.com/xxN2isv</a><br>
<br>
At the top, you see the signature(s) viable for the call. Below you get the<br>
list of code completion offers, and the "best matches" are obtained from<br>
sorting these results such that the types match the viable call signatures.<br>
<br>
The screenshots above are obtained from running our legacy C++ code model. We<br>
are currently working on a clang-based code model and that already gives us<br>
similar results, including the sorting. What we are missing so far is the<br>
information to display on top for the possible viable call signatures. We can<br>
probably workaround that somehow, but having a Clang(-c) API for that would be<br>
nice.<br>
<br>
If I'm not mistaking your video, your patch removes the list of results and<br>
replaces it only by a list of matching types? This would be an unfortunate<br>
regression, imo.<br>
<br>
Bye<br>
<span class=""><font color="#888888">--<br>
Milian Wolff<br>
<a href="mailto:mail@milianw.de">mail@milianw.de</a><br>
<a href="http://milianw.de" target="_blank">http://milianw.de</a><br>
</font></span></blockquote></div><br></div></div>