bug#34525: replace-regexp missing some matches

[Alan Mackenzie]

Hello, Eli and Stefan.

On Wed, Feb 27, 2019 at 20:07:50 +0200, Eli Zaretskii wrote:
> > Date: Wed, 27 Feb 2019 17:31:32 +0000
> > Cc: monnier@IRO.UMontreal.CA, daniel.lopez999@gmail.com, 
> > 34525@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > next_interval and previous_interval are used extensively, so I'm
> > > having hard time believing that they have such a blatant bug.

> > Yes, how come we haven't seen many more consequences?

> > Maybe syntax-table text properties aren't as widely used as all that.

> I actually had the text properties in mind.  They are used all over
> the place.  Faces are a feature that would make any such bugs
> immediately visible.

I think the mechanism with update_interval is only used in syntax.c, and
only for the syntax-table property.

> > I'm guessing this fix will be deemed too unsafe to make it into the
> > emacs-26 release branch.  ;-(

> That goes without saying.

OK.  Progress on the bug seems to have stalled somewhat.  Now that we
understand the cause, one of us needs to decide how to fix it.  I think
we've discussed the following alternatives:

(i) Calculate ->position's in previous_interval and next_interval, as my
  tentative patch already does.
(ii) Calculate the ->position's in update_interval, on moving to
  parents.
(iii) Do away with update_interval, replacing it in syntax.c with
  previous/next_interval in while loops.

At the moment, only (i) has been tried.

Speed-wise, it seems not to make any difference in an optimised GNU
build, though it did appear to be significantly (~4%) slower on an
unoptimised build which scrolls through a C++ file with lots of
templates.  I don't think it's worth the effort to make a systematic
speed comparison between the alternatives.

(iv) Additionally, there is a cleanup wanted, where setting ->position
  in the chain of parents should be moved from update_syntax_table to
  find_interval.

In (i), the convention for ->position would be that it is valid for the
target interval together with all its parents.  In (ii) and (iii), it
would only be valid in the final target intervals found by navigation.
I think this should be explicitly stated in a comment in struct
interval.

So, where do we go from here?  If it were up to me, I would probably
chose (i), simply because it's already been done, but I've no strong
feelings over it.  I'm also willing to implement (ii), or even (iii), if
that is wanted.  In any case, I would ask one of you for a careful code
review of my changes - this stuff is easy to get wrong.

-- 
Alan Mackenzie (Nuremberg, Germany).