Sorting completion candidates based on clang priority

[434189275]

Sorting completion candidates based on clang priority

[Sarcasm]

Hello and thank you for proposing this change!

Candidates are currently sorted according to clang_sortCodeCompletionResults(), to benefit from the performances of letting libclang doing it.
And personally, I prefer the predictability of alphabetical ordering.

Some frontends may like to sort them that way, e.g. when merging candidates from multiple backends.

I do expose the priority to elisp, so that the frontend (e.g. company-irony) can sort accordingly.
So, if you use the company frontend, I would suggest you to make a patch there, and enable it only under a defcustom.

[434189275]

Hello and very nice mode by the way! Keep up the good work!

I personally use it mainly for C++ with company-irony, and the reason why I submitted this pull request is that sorting according to libclang priority (clang_getCompletionPriority()) allows completions to be context-aware, and thus, more relevant. Take for example this situation:

In that case, I think most users would expect to have the sprite variable in first position, sprintf being less relevant here.

Now, sorting according to libclang priority:

sprite variable now in first position, results still being sorted alphabetically.

And personally, I prefer the predictability of alphabetical ordering.

I totally agree with you on that point, but I think you'll agree that alphabetical sorting should not prevail on the relevance of the completion.

As you see, I could have just removed the call to clang_sortCodeCompletionResults() to have results sorted according to priority only, but as I, too, want the results to be sorted alphabetically, I manually sort them after calling clang_sortCodeCompletionResults() so we have both the benefits of the clang priority and of the alphabetical sorting.

What do you think ?

[Sarcasm]

I do think it's a legitimate use case, but I think it can be done in company-irony, in a defcustom variable, so that it's easier to enable/disable than something in the C++ part.

I believe you can wrap this around company-irony--candidates:

https://github.com/Sarcasm/company-irony/blob/52aca45bcd0f2cb0648fcafa2bbb4f8ad4b2fee7/company-irony.el#L78

Do you see what I mean?

[434189275]

I get what you mean, but I definitely think that sorting should not be done in the front end for obvious performance reasons. More over, it can very easily be disabled by the mean of a defcustom variable, passed to the candidates command along with the prefix and style arguments.

Now, I do not see the need for disabling it, as totally ignoring clang priority actually leads up to incorrect completions in my opinion. And as I said above, alphabetical sorting is not put apart, it is just improved by taking into account the clang priority.

[Sarcasm]

Sorting in C++ could be faster, but sorting in company, assuming the default prefix length of size 3, does not sort all the candidates, but only a subset.
So in the general case, I'm not sure of how slower it would be, could really depend on the prefix.

Ok for prefix and style-like method, I would accept such PR.
I reopen if you prefer this solution.

[Sarcasm]

Maybe do like PrefixMatchStyle, and make a dedicated enum to represent the possibles values instead of storing it as a string.

[Sarcasm]

I think it could make more sense to do this in the complete command, instead of candidates.
complete is called once but candidates many time.
Since the sorting does not depend on the candidates input, it may be better to do the sorting once, next to the sorting done by clang.

[Sarcasm]

Would be nice to modify Command.def as well, to change the description of the command (which I recommend below to apply to the complete command, instead of candidates).

[Sarcasm]

In terms of vocabulary, I think I would do something like:

sorting-method: priority, alphabetical.

I would not allow nil, but instead produce an error.
nil seems to imply this is the soundest default value, which I am not yet sure.
A more neutral approach is to have enumeration value, and if one make more sense than the other, the defcustom will be updated accordingly.
I'm mostly thinking about a future 3rd option coming up, that would be the best default value.