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 #40 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 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: Sat, 23 Aug 2025 10:53:20 -0400
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> So how about this? This mostly reverts my earlier patch, so it may be
>> easier to review it relative to the old version before my earlier patch.
>
> Now I'm wondering what is the advantage of this new code compared to the
> old code.
Two things:
- It should have somewhat better performance because we're running
try-completion on smaller strings and therefore doing less string
comparison and work.
- It gets rid of the mutable variable FIXED which is mutated across each
iteration of the loop over the pattern. IMO this makes the code much
easier to follow.
Below is the diff from the original version, before my already-installed
earlier patch; possibly this is easiest to review.
>> * lisp/minibuffer.el (completion-pcm--pattern->regex): When
>> GROUP is group-all, also group strings in PATTERN.
>
> Why do we need that?
It makes it possible to extract the parts of the completions that
correspond to each part of PATTERN, rather than just the parts
corresponding to wildcards.
CCS previously contained, for each completion, a list containing one
element for each wildcard element of PATTERN.
Now CCS contains, for each completion, a list containing one element for
each element of PATTERN, both fixed strings and wildcards.
(This lets us iterate over PATTERN and CCS with a more uniform loop -
there could probably even be some further simplifications, to zip the
two lists together one way or another)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index b5060614841..37bfbdc0765 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4187,7 +4207,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4586,7 +4610,7 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (completion-pcm--pattern->regex pattern 'group))
+ (let ((re (completion-pcm--pattern->regex pattern 'group-all))
(ccs ())) ;Chopped completions.
;; First chop each string into the parts corresponding to each
@@ -4610,36 +4634,35 @@ completion-pcm--merge-completions
;; Then for each of those non-constant elements, 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))
- (setq wildcards nil))
- (let ((comps ()))
+ (let ((comps ()))
+ (dolist (cc (prog1 ccs (setq ccs nil)))
+ (push (car cc) comps)
+ (push (cdr cc) ccs))
+ ;; Might improve the likelihood to avoid choosing
+ ;; different capitalizations in different parts.
+ ;; In practice, it doesn't seem to make any difference.
+ (setq ccs (nreverse ccs))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (push (cond ((eq shared t) elem)
+ ((stringp shared) shared)
+ (t ""))
+ res)
+ (setq wildcards nil))
(push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; 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.
@@ -4654,7 +4677,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.
@@ -4678,8 +4701,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)))))
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.