Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler <mail@daniel-mendler.de>]

On 6/1/21 5:49 PM, João Távora wrote:
> But do you have any evidence to suggest that making a cons and
> allocating a function is detrimental to this??  I just tried this small
> patch to the function you suggested as a potential hog, since it
> iterates all the completions.
>
> So basically the same.  Cons and lambda's are cheap.

The allocations itself are cheap but they put unnecessary pressure on
the GC. The current version avoids this. This matters when scrolling
through long candidate lists.

>> The current implementation is simple enough and also avoids allocation
>> of the cons pair and the allocation of the lambda as in your proposal.
>> The efficiency is crucial here.
> 
> I believe you, but I haven't seen any evidence (yet) to back up the
> claim.  But instead of me whistling premature optimization song, feel
> free to point me to some numbers or something.

The current version is proven in my packages and performs well. I don't
see a point in replacing it to a more complicated and less efficient
version.

>> Furthermore I argue that the current implementation is simpler to
>> understand for the completion table authors than your counter
>> proposal, which introduces a deferred computation with the lambda.
> 
> It's like the affixation-function thing, I don't think it makes sense
> for client code to do these IFs, much as I dont' think it makes sense
> for them to do MAPCARs.

No it is not. It is more like the decoration function of which you are
also in favor. The type is this:

group-function : string -> bool -> string

The bool corresponds to the fields to return in the decoration function.
Either return the title or return the transformed candidate.

> But to be clear, the main advantage here in my opinion is the cleanup in
> minibuffer.el, which is somehwat messy in my opinion (not only from
> group-fn's fault, of course).

That's the wrong way to look at it. The main thing which should be
optimized here is the performance and the ease of usage for the
completion table authors. Furthermore I disagree with you that the
minibuffer.el code is messy. The same applies to the Vertico code, where
I implemented group function support. It is also reasonably clean.
Please take a look at it.

The suggestion of yours to mutate the candidates by putting a property
instead of threading the group function through the call sites is much
worse.

>> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
>> multiple different designs for the `group-function` have been considered
>> and we settled on the current design.
> 
> It could have been discussed with your favourite divinity, for all I
> care.  If I see something worthy of improvement for the next version,
> I'm going to suggest it here.  Especially in the case of a new feature
> in master. "It's been discussed with so and so" is not a worthless
> argument, but it's not very valuable either.

Your version is not an improvement. Please read through the old
discussion and consider my arguments. I also don't see a point in
rediscussing this as long as you don't make a proposal which is a clear
improvement.

Daniel