> From: Stefan Monnier <monn...@iro.umontreal.ca> > Cc: Robert Pluim <rpl...@gmail.com>, 23...@debbugs.gnu.org, > alex.ben...@linaro.org, jwieg...@gmail.com, nljlistb...@gmail.com > Date: Mon, 18 Jul 2016 20:58:35 -0400 > > > In the case in point, a single character at EOB (= 62) was deleted, > > which made EOB be 61, one less than its previous value. When > > save-match-data was called from within a hook set up by Org, it tried > > to record the end of the sub-expression as 62, but set-marker silently > > changed that to 61. That "corrected" value was subsequently restored > > when save-match-data was exited, whereas replace-match expected to see > > the original value of 62, and therefore barfed. > > I think this change performed by save-match-data is harmless: the old > value (62) was not valid any more anyway.
In this particular case, yes. But only in this case, because (a) there's actually only one sub-expression, and (b) it ends exactly at EOB. The more general problem is when there's at least one more sub-expression, whose start and/or end are after the new EOB. Those sub-expression's data will be completely bogus after the adjustment, should the buffer-modification hooks use save-match-data. > So I think a safe fix is to try and relax the check we added to > replace-match so it doesn't get all worked up when something ≥ EOB gets > changed to something else that's also ≥ EOB. And lose the other sub-expressions in a more general case? Really? > Or maybe instead of signaling an error, we could simply skip the "Adjust > search data for this change". That would still sweep the problem under the carpet, leaving the match data bogus, so I don't like doing that. > This said, I don't fully understand what's going on: bug#23869 reported > a crash, but AFAICT the match-data here is only used to adjust > search_regs which seems like it wouldn't cause a crash, even if the new > values are bogus. The crash in bug#23869 was due to this: newpoint = search_regs.start[sub] + SCHARS (newtext); [...] /* Now move point "officially" to the start of the inserted replacement. */ move_if_not_intangible (newpoint); <<<<<<<<<<<<<<<<<<<<<<< because due to clobbering, newpoint became -1. > > - '((save-match-data-internal (match-data))) > > + '((save-match-data-internal (match-data 'integers))) > > That looks risky. Then how about manually doing the equivalent of save-match-data around the call to replace_range, calling match-data with non-nil argument?
