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.