[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#67124: 26.3; query-replace Arg out of range with comma option (at en
From: |
Eli Zaretskii |
Subject: |
bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer) |
Date: |
Sat, 18 Nov 2023 12:18:12 +0200 |
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: gabriele@medialab.sissa.it, 67124@debbugs.gnu.org
> Date: Thu, 16 Nov 2023 13:23:48 -0500
>
> >> --- a/src/search.c
> >> +++ b/src/search.c
> >> @@ -3142,9 +3142,16 @@ update_search_regs (ptrdiff_t oldstart, ptrdiff_t
> >> oldend, ptrdiff_t newend)
> >>
> >> for (i = 0; i < search_regs.num_regs; i++)
> >> {
> >> - if (search_regs.start[i] >= oldend)
> >> + if (search_regs.start[i] <= oldstart)
> >> + /* If the subgroup that `replace-match` is modifying encloses the
> >> + subgroup `i`, then its `start` position should stay unchanged.
> >> + That's always true for subgroup 0.
> >
> > I've read this part of the comment many times, and I still don't
> > understand what you are trying to convey there, and thus don't
> > understand the new 'if' clause you added. In particular, how come
> > this was never something brought to our attention? the comment seems
> > to imply that replace-match was somehow badly broken since forever.
>
> `replace-match` goes through the trouble of trying to keep the
> match-data valid by moving the subgroup positions.
>
> The way it does it currently is sometimes flat out incorrect and other
> times debatable. For end positions, we do:
>
> if (search_regs.end[i] >= oldend)
> search_regs.end[i] += change;
> else if (search_regs.end[i] > oldstart)
> search_regs.end[i] = oldstart;
>
> which means that positions beyond (or at the end of) the change are
> moved appropriately, positions before (or at the beginning of) the change
> are left untouched and positions strictly within the change are moved to
> the beginning of the change.
> If the position is both at start and end at the same time, we move them,
> assuming that they're more at the end than at the start. For subgroup
> 0, this is definitely correct. For other subgroups, it depends on the
> inclusion or relative position of that subgroup with the subgroup that
> `replace-match` is replacing. E.g. if I matched
> "\\(\\)\\(\\(\\)\\)\\(\\)" and I replace subgroup 3 with "hi", then the
> ideal behavior would be to move the end of subgroups 0, 2, 3, and 4 but
> not 1.
>
> For start positions, we do the same:
>
> if (search_regs.start[i] >= oldend)
> search_regs.start[i] += change;
> else if (search_regs.start[i] > oldstart)
> search_regs.start[i] = oldstart;
>
> When oldstart < oldend this works OK, but when they're equal, moving the
> start position is definitely wrong for subgroup 0 and is often wrong for
> others as well. In the above regexp example, we should ideally move the
> start position of subgroup 4 only and we should leave the other
> ones unchanged.
>
> `update_search_regs` doesn't have enough information to figure out which
> subgroup is before/after/around/within some other subgroup and it isn't
> even told which subgroup is being updated by `replace-match`, so the
> best it can do is to either move them all or move none. The current
> code moves them all, which is always wrong for start[0].
> My patch intends to make the other choice so it will get it wrong for
> start[4] in the above example (just like the current code gets it wrong
> for end[1]) but it will get it right for the other ones.
Please include at least some, if not all, of this extended explanation
in the comments there.
And I think you can install your changes on master.
Thanks.
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), (continued)
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer),
Eli Zaretskii <=
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/18
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/18
- Message not available
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Dmitry Gutov, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Dmitry Gutov, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Gabriele Nicolardi, 2023/11/13
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Stefan Monnier, 2023/11/16
- bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Gabriele Nicolardi, 2023/11/16
bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer), Eli Zaretskii, 2023/11/15