bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Drew Adams]

Because of this part, at the end of the function,
`completion-pcm-all-completions' reverses the order of the candidates.

(let ((poss ()))
  (dolist (c compl)
    (when (string-match-p regex c) (push c poss))) ; <==== reverses
  poss)

It should not do this.  Users should be able to depend on whatever order
the candidates are already in (as returned by `all-completions').

For example, for buffer candidates, the default order from
`all-completions' is last-display-time order.

The fix is trivial:

(let ((poss ()))
  (dolist (c compl)
    (when (string-match-p regex c) (push c poss)))
  (nreverse poss))


In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-09-17
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --without-compress-install CFLAGS=-static'

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Michael Heerdegen]

Hello,

> Because of this part, at the end of the function,
> `completion-pcm-all-completions' reverses the order of the candidates.
>
> (let ((poss ()))
>   (dolist (c compl)
>     (when (string-match-p regex c) (push c poss))) ; <==== reverses
>   poss)
>
> It should not do this.  Users should be able to depend on whatever order
> the candidates are already in (as returned by `all-completions').
>
> For example, for buffer candidates, the default order from
> `all-completions' is last-display-time order.
>
> The fix is trivial:
>
> (let ((poss ()))
>   (dolist (c compl)
>     (when (string-match-p regex c) (push c poss)))
>   (nreverse poss))

Ah, this is the reason for the candidate list reversion I see.  Yes,
this should IMO not happen.

Stefan, if you get the time, can you please have a look at whether the
suggested fix looks ok to you, and commit it if applicable?


Thanks,

Michael.

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Stefan Monnier]

> Because of this part, at the end of the function,
> `completion-pcm-all-completions' reverses the order of the candidates.

completion-pcm-all-completions doesn't have any such code.
After searching a bit I decided you meant completion-pcm--all-completions.

> (let ((poss ()))
>   (dolist (c compl)
>     (when (string-match-p regex c) (push c poss)))
>   (nreverse poss))

Looks good, please install,


        Stefan

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> completion-pcm-all-completions doesn't have any such code.  After
> searching a bit I decided you meant completion-pcm--all-completions.

Oh yes, I found that too, but forgot to mention.

>
> > (let ((poss ()))
> >   (dolist (c compl)
> >     (when (string-match-p regex c) (push c poss)))
> >   (nreverse poss))
>
> Looks good, please install,

This doesn't seem to meet any of the criteria in

    http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html

so I think I'll have to commit to master.


Michael.

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Drew Adams]

> > > (let ((poss ()))
> > >   (dolist (c compl)
> > >     (when (string-match-p regex c) (push c poss)))
> > >   (nreverse poss))
> >
> > Looks good, please install,
> 
> This doesn't seem to meet any of the criteria in
>     http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html
> so I think I'll have to commit to master.

Thanks for fixing this, Michael.

Unfortunately, there is no way for my code that compensates for this bug to now know whether/when it has been fixed.  No simple test such as `fboundp' or an Emacs version.

Of course, if you happened to decide to rename the function, there would be no problem...

And it's already a bit much that there is also a function `completion-pcm-all-completions' that does not even call `completion-pcm--all-completions', and that has a completely different signature.  And the "internal" function has a nice doc string, but the "external" one has no doc string...

Perhaps you would consider taking this opportunity to find a better name for this "internal" function?  Especially since it is "internal", probably few 3rd-party libraries would be affected by a name change.

In my case, I would not be referencing this "internal" function at all, except that I needed to work around this bug.  If you did choose to rename it, I would not need to make any code changes, because I already protect this change with `(when (fboundp 'completion-pcm--all-completions)...)'.

bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> Unfortunately, there is no way for my code that compensates for this
> bug to now know whether/when it has been fixed.  No simple test such
> as `fboundp' or an Emacs version.
>
> Of course, if you happened to decide to rename the function, there
> would be no problem...

Hmm, that sounds reasonable.  If nobody objects, I can do that, sure.


Michael.