GNU bug report logs - #79265
[PATCH] Treat point more consistently in PCM completion

Previous Next

Full log

Message #52 received at 79265 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
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,
 [PATCH] Refactor completion-pcm--merge-completions
Date: Mon, 01 Sep 2025 01:41:37 -0400

>> Attached is a patch (which applies on master) which does most of the
>> rest of the refactoring that I wanted to do; it makes the dolist over
>> the pattern into a simple mapcan over the pattern, with no state
>> preserved between elements of the pattern.  IMO, this is much easier to
>> follow (and I think it would make the various bugs I've fixed in the
>> last couple years in merge-completions, easier to catch).  What do you
>> think?

I still don't understand what this is trying to do.  It still seems more
costly since it allocates more strings and conses onto
`pattern-and-comps` than the previous code did with `ccs` (the
"corresponding" variable).

Also, can we just revert the last patch (and add the test(s) that we
found in the mean time) so as to make it easier to review patches?

> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -4249,7 +4249,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 "*?")

I think we should aim for a patch which doesn't use that many subgroups.

> @@ -4622,6 +4626,19 @@ completion--common-suffix
>    "Return the common suffix of the strings STRS."
>    (nreverse (try-completion "" (mapcar #'reverse strs))))
>  
> +(defun completion-pcm--group-pattern (pattern)
> +  "Group together adjacent wildcards in PATTERN."
> +  (let (ret)
> +    (dolist (elem pattern)
> +      (cond
> +       ((or (stringp elem) (eq elem 'any-delim))
> +        (push elem ret))
> +       ((consp (car-safe ret))
> +        (setf (car ret) (append (car ret) (list elem))))
> +       (t
> +        (push (list elem) ret))))
> +    (nreverse ret)))

Why is `any-delim` not considered a "wildcard"?

Maybe a better grouping would be to turn

    ("a" "c" any "b" point)

into something like

    (("a" "c" any) ("b" point))

or

    (("ac" any) ("b" point))

where each sublist will get its own regexp subgroup and will have its
corresponding `try-completion`.

> +      ;; Then for each of those non-constant elements, extract the
> +      ;; commonality between them.

AFAICT this includes "constant" elements, so either I'm missing
something or the comment is off.

> @@ -4774,7 +4773,7 @@ completion-pcm--merge-try
>           (equal (completion-pcm--pattern->string pattern) (car all)))
>      t)
>     (t
> -    (let* ((mergedpat (completion-pcm--merge-completions all pattern))
> +    (let* ((mergedpat (nreverse (completion-pcm--merge-completions all pattern)))
>             ;; `mergedpat' is in reverse order.  Place new point (by
>             ;; order of preference) either at the old point, or at
>             ;; the last place where there's something to choose, or

Shouldn't that require an update to the subsequent comment?
Or is it that `completion-pcm--merge-completions` now doesn't return the
merged pat in reverse order, so we need an explicit `nreverse` to
recover the previous reserve-order merged pat?


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.