Package: emacs;
Reported by: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 18:54:01 UTC
Severity: normal
Tags: patch
Done: Dmitry Gutov <dmitry <at> gutov.dev>
Message #20 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Wed, 20 Aug 2025 13:26:16 -0400
[Message part 1 (text/plain, inline)]
Spencer Baugh <sbaugh <at> janestreet.com> writes: > Dmitry Gutov <dmitry <at> 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.
[0001-Avoid-duplicating-strings-in-pcm-merge-completions.patch (text/x-patch, inline)]
From fcaab60e765961b2fffe064c1facf078bce1a101 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Wed, 20 Aug 2025 13:23:34 -0400
Subject: [PATCH] Avoid duplicating strings in pcm--merge-completions
Make completion-pcm--merge-completions operate only on the text
matched by the wildcards, instead of also the text in between
the wildcards. This improves performance and simplifies the
code by removing the need for the previous mutable variable
"fixed".
* lisp/minibuffer.el (completion-pcm--merge-completions):
Operate only on text matched by wildcards. (bug#79265)
---
lisp/minibuffer.el | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 36202566136..999d699c488 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4706,38 +4706,35 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (completion-pcm--pattern->regex pattern 'group))
+ (let ((re (concat
+ (completion-pcm--pattern->regex pattern 'group)
+ ;; The implicit trailing `any' is greedy.
+ "\\([^z-a]*\\)"))
(ccs ())) ;Chopped completions.
- ;; First chop each string into the parts corresponding to each
- ;; non-constant element of `pattern', using regexp-matching.
+ ;; First match each string against PATTERN as a regex and extract
+ ;; the text matched by each wildcard.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
(let ((chopped ())
- (last 0)
(i 1)
next)
- (while (setq next (match-end i))
- (push (substring str last next) chopped)
- (setq last next)
+ (while (setq next (match-string i str))
+ (push next chopped)
(setq i (1+ i)))
- ;; Add the text corresponding to the implicit trailing `any'.
- (push (substring str last) chopped)
(push (nreverse chopped) ccs))))
- ;; Then for each of those non-constant elements, extract the
- ;; commonality between them.
+ ;; Then for each of those wildcards, extract the commonality between them.
(let ((res ())
- (fixed "")
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
(if (stringp elem)
(progn
- (setq fixed (concat fixed elem))
+ (push elem res)
(setq wildcards nil))
(let ((comps ()))
(push elem wildcards)
@@ -4748,18 +4745,13 @@ completion-pcm--merge-completions
;; different capitalizations in different parts.
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
- ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
- (let* ((prefix (try-completion fixed comps))
- (unique (or (and (eq prefix t) (setq prefix fixed))
+ (let* ((prefix (try-completion "" comps))
+ (unique (or (and (eq prefix t) (setq prefix ""))
(and (stringp prefix)
;; If PREFIX is equal to all of COMPS,
;; then PREFIX is a unique completion.
(seq-every-p
- ;; PREFIX is still a prefix of all of
- ;; COMPS, so if COMP is the same length,
- ;; they're equal.
- (lambda (comp)
- (= (length prefix) (length comp)))
+ (lambda (comp) (= (length prefix) (length comp)))
comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
@@ -4774,7 +4766,7 @@ completion-pcm--merge-completions
;; `prefix' only wants to include the fixed part before the
;; wildcard, not the result of growing that fixed part.
(when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix fixed))
+ (setq prefix ""))
(push prefix res)
;; Push all the wildcards in this stretch, to preserve `point' and
;; `star' wildcards before ELEM.
@@ -4798,8 +4790,7 @@ completion-pcm--merge-completions
(unless (equal suffix "")
(push suffix res))))
;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))
- (setq fixed "")))))
+ (setq wildcards nil))))))
;; We return it in reverse order.
res)))))
--
2.43.7
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.