Spencer Baugh <sbaugh@janestreet.com> writes:

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
>> On 19/08/2025 19:03, Spencer Baugh wrote:
>>> Hm, good point.  After thinking about it more, I realized that the
>>> second try-completion call above somewhat duplicates the check I added,
>>> and that I could refactor in general to improve performance here.
>>> So I reworked the change a bit to combine my check with the second
>>> try-completion call.  This should have better performance even though we
>>> aren't using the C implementation of try-completion anymore, since we're
>>> now doing much less work than try-completion does.
>>> (I also refactored to use a cond instead of (or (and ...) (and
>>> ...)),
>>> which I think makes this logic a fair bit clearer)
>>
>> Hi, this looks sensible. Just a couple of questions.
>>
>>> -                     (unique (or (and (eq prefix t) (setq prefix fixed))
>>> -                                 (and (stringp prefix)
>>> -                                      (eq t (try-completion prefix comps))))))
>>> +                     (unique
>>> +                      (cond
>>> +                       ((eq prefix t)
>>> +                        (setq prefix fixed)
>>> +                        t)
>>> +                       ((stringp prefix)
>>> +                        ;; `try-completion' is almost this, but it doesn't
>>> +                        ;; handle `completion-ignore-case' the way we want.
>>> +                        (not (cl-some
>>
>> This gets a warning "the function ‘cl-some’ might not be defined at
>> runtime" because cl-lib is only loaded for macros, in minibuffer.el.
>
> Oops, sorry, just missed that.
>
>> So IDK - maybe it's better to use the alien cl-loop, or maybe to go
>> back to seq-some, if no measurements show a cause for worry.
>
> Eh, I went back to seq-every-p for now.  This shouldn't really cause a
> performance problem, since it will usually just return nil quickly.
> Also it's only run when the user hits TAB.
>
> (I'd rather not use cl-loop, this code is already hard enough to
> understand :) )
>
>>> +                              (lambda (comp)
>>> +                                ;; PREFIX could complete to COMP, so it's not unique.
>>> +                                (and (string-prefix-p prefix comp completion-ignore-case)
>>> +                                     (not (= (length prefix) (length comp)))))
>>> +                              comps))))))
>>
>> Just to check: in our limited test examples all elements of COMP match
>> PREFIX already. And the beginning of
>> 'completion-pcm--merge-completions' even 'error'-s if string-match
>> comparison fails.
>>
>> So it's possible that we could omit this comparison from the lambda:
>>
>>   (and (string-prefix-p prefix comp completion-ignore-case)
>>
>> But I suppose that would make it a bigger change, a bit riskier.
>
> Ah, you're right.  I missed that we already have this property
> guaranteed, but indeed we do.
>
> So thanks to that, the change can be further simplified.  Removing my
> cond refactoring, the patch is now just replacing
>
>   (eq t (try-completion prefix comps))
>
> with
>
>   (seq-every-p
>     (lambda (comp) (= (length prefix) (length comp)))
>     comps)
>
> which is a nice simple change.  I added some comments to clarify why
> this is correct, too.

And this followup patch makes it much clearer that this is correct, and
also improves performance further.  I think we should apply both.