unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
Cc: 42149@debbugs.gnu.org, "João Távora" <joaotavora@gmail.com>
Subject: bug#42149: Substring and flex completion ignore implicit trailing ‘any’
Date: Sun, 27 Dec 2020 16:45:07 -0500	[thread overview]
Message-ID: <jwvim8nhrs5.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <fv2zojh7qvy3az.fsf@gmail.com> (Dario Gjorgjevski's message of "Thu, 15 Oct 2020 16:25:08 +0200")

> Please find attached a patch with an edited commit message.  It should
> be better.

[ Boy, we are really unacceptably slow at reviewing this.  ]

Thanks very much for your patch.
It looks good to me, but I think it's important we find a fix with which
João agrees.

> Furthermore, ‘completions-first-difference’ and
> ‘completions-common-part’ would sometimes overlap depending on the
> position of point within the query string.

Could you point us at the corresponding test?
[ And thanks so much for the tests, this is great!  ]

> The former is fixed by correcting the part of
> ‘completion-pcm--hilit-commonality’ responsible for looping over the
> holes in the query string.

Is that done by treating the "leftover" from the string as if there was
an additional match?  That would correspond to the "implicit any"
that terminates every pattern.

> The latter is fixed by explicitly moving
> the position of ‘completions-first-difference’ in case an overlap with
> ‘completions-common-part’ is detected.

Did you (by any chance) figure out how/why the two end up overlapping?
The fix you're using looks pretty "hackish" and introduces a non-trivial
data flow for `pos`.  Before using such an ad-hoc solution it'd be best
to understand where the problem comes from (it might still be the
better answer in the end, but it's hard to judge).

> (completion-pcm--optimize-pattern): Turn multiple consecutive
> occurrences of ‘any’ into just a single one.

This is good (consecutive `any` can introduce serious performance bugs
because of our backtracing regexp matcher).
Other than improving performance, have you found other effects?

> +(defun completion-pcm--count-leading-holes (pattern)
> +  "Count the number of leading holes in PATTERN."
> +  (length (seq-take-while #'symbolp pattern)))

`seq-take-while` is not defined at this stage.
Either:
- (require 'seq), but that means `seq` will have to be preloaded,
  which will require negotiating with Eli.
- Mark `seq-take-while` with a `;;;###autoload` cookie so it'll be
  loaded on demand.
- Avoid using `seq-take-while` here.
I vote for the the 2nd option.

I think João knows the scoring algorithm much more than I do, so I'll
let him judge if the change is sound.


        Stefan






  parent reply	other threads:[~2020-12-27 21:45 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-01 10:40 bug#42149: Substring and flex completion ignore implicit trailing ‘any’ Dario Gjorgjevski
2020-07-01 10:58 ` João Távora
2020-07-01 11:03 ` João Távora
2020-07-01 11:10   ` Dario Gjorgjevski
2020-09-08  9:05     ` Dario Gjorgjevski
2020-09-08  9:30       ` João Távora
2020-09-08  9:44         ` Dario Gjorgjevski
2020-09-08 10:08           ` João Távora
2020-09-08 11:12             ` Dario Gjorgjevski
2020-09-08 11:22               ` João Távora
2020-09-08 11:30                 ` Dario Gjorgjevski
2020-09-08 11:32                   ` João Távora
2020-09-09 10:17                     ` Dario Gjorgjevski
2020-09-09 11:38                       ` Dario Gjorgjevski
2020-09-09 13:13                         ` Stefan Monnier
2020-09-10 11:26                           ` Dario Gjorgjevski
2020-10-14  8:22                             ` Dario Gjorgjevski
2020-10-14  8:39                               ` João Távora
2020-10-14  9:01                                 ` Dario Gjorgjevski
2020-10-15 14:25                                   ` Dario Gjorgjevski
2020-11-20 20:39                                     ` Dario Gjorgjevski
2020-11-20 21:27                                       ` João Távora
2020-11-25  0:01                                         ` João Távora
2020-11-25  8:22                                           ` Dario Gjorgjevski
2020-11-25 12:22                                             ` João Távora
2020-11-25 13:27                                               ` Dario Gjorgjevski
2020-12-23  9:41                                                 ` Dario Gjorgjevski
2020-12-27 20:08                                                   ` João Távora
2020-12-27 20:23                                                     ` João Távora
2020-12-27 21:20                                                     ` Stefan Monnier
2020-12-28  9:30                                                       ` João Távora
2020-12-28 16:03                                                         ` Stefan Monnier
2020-12-28 16:58                                                           ` João Távora
2020-12-28 16:07                                                         ` Stefan Monnier
2020-12-28 17:04                                                           ` João Távora
2020-12-27 21:45                                     ` Stefan Monnier [this message]
2020-12-28  9:38                                       ` João Távora
2020-12-28 10:22                                         ` Dario Gjorgjevski
2020-12-28 11:34                                           ` João Távora
2020-12-28 11:48                                             ` Dario Gjorgjevski
2020-12-28 12:57                                               ` João Távora
2020-12-28 10:17                                       ` Dario Gjorgjevski
2020-12-28 16:26                                         ` Stefan Monnier
2020-12-28 17:16                                           ` João Távora
2020-12-28 19:48                                             ` Dario Gjorgjevski
2020-12-28 20:00                                               ` Stefan Monnier
2020-12-28 23:20                                                 ` João Távora
2020-12-29 13:27                                                   ` João Távora
2021-05-13  9:24                                                   ` Lars Ingebrigtsen
2021-05-13 14:31                                                     ` João Távora
2021-05-13 15:41                                                       ` Dario Gjorgjevski
2021-05-13 16:04                                                         ` João Távora
2021-05-16 13:51                                                           ` Lars Ingebrigtsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvim8nhrs5.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=42149@debbugs.gnu.org \
    --cc=dario.gjorgjevski@gmail.com \
    --cc=joaotavora@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).