Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Stefan Monnier]

> -      prepare_to_modify_buffer (from, to, NULL);
> +      /* If k >= l, it means nothing needs to be deleted.  */
> +      if (k < l)
> +	prepare_to_modify_buffer (from, to, NULL);

This comment makes it sound like before-change-functions won't be run
when there are only insertions (which would be wrong).

> -      signal_after_change (from, to - from, updated_to - from);
> -      update_compositions (from, updated_to, CHECK_INSIDE);
> +      /* Only call after-change-functions if something was actually
> +	 inserted.  */
> +      if (from < updated_to)
> +	{
> +	  signal_after_change (from, to - from, updated_to - from);
> +	  update_compositions (from, updated_to, CHECK_INSIDE);
> +	}

And this comment makes it sound like after-change-functions won't be run
when there are only deletions (which would be similarly wrong).


        Stefan

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 21 Jul 2018 14:16:29 -0400
> 
> > -      prepare_to_modify_buffer (from, to, NULL);
> > +      /* If k >= l, it means nothing needs to be deleted.  */
> > +      if (k < l)
> > +	prepare_to_modify_buffer (from, to, NULL);
> 
> This comment makes it sound like before-change-functions won't be run
> when there are only insertions (which would be wrong).
> 
> > -      signal_after_change (from, to - from, updated_to - from);
> > -      update_compositions (from, updated_to, CHECK_INSIDE);
> > +      /* Only call after-change-functions if something was actually
> > +	 inserted.  */
> > +      if (from < updated_to)
> > +	{
> > +	  signal_after_change (from, to - from, updated_to - from);
> > +	  update_compositions (from, updated_to, CHECK_INSIDE);
> > +	}
> 
> And this comment makes it sound like after-change-functions won't be run
> when there are only deletions (which would be similarly wrong).

I don't understand your claims, sorry.  Is the code wrong, or are the
comments wrong?  And what would you expect to see in the buffer
modification hooks calls for the recipe of that bug?

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Stefan Monnier]

> I don't understand your claims, sorry.  Is the code wrong, or are the
> comments wrong?

I don't understand enough of the code to have an opinion on it, but the
comments describe a behavior which would be wrong: both before-c-f and
after-c-f- need to be run for any buffer change, even if it's only an
insertion or only a deletion.

> And what would you expect to see in the buffer
> modification hooks calls for the recipe of that bug?

Lots of options, but basically: one call to b-c-f with FROM being < then
the first position at which a deletion or insertion will take place and
TO being after the last such position (e.g. from==to if the change is
a single insertion); followed by one call to a-c-f with the
same constraints (e.g. from==to if it's a single deletion).


        Stefan

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 21 Jul 2018 14:29:27 -0400
> 
> > I don't understand your claims, sorry.  Is the code wrong, or are the
> > comments wrong?
> 
> I don't understand enough of the code to have an opinion on it, but the
> comments describe a behavior which would be wrong: both before-c-f and
> after-c-f- need to be run for any buffer change, even if it's only an
> insertion or only a deletion.

What if there's no change at all, i.e. no deletions and no insertions?
That was the OP's recipe.

> > And what would you expect to see in the buffer
> > modification hooks calls for the recipe of that bug?
> 
> Lots of options, but basically: one call to b-c-f with FROM being < then
> the first position at which a deletion or insertion will take place and
> TO being after the last such position (e.g. from==to if the change is
> a single insertion); followed by one call to a-c-f with the
> same constraints (e.g. from==to if it's a single deletion).

You did read the bug report, didn't you?  Because unless I completely
misunderstand what you are saying, you are not describing the bug's
recipe.

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Stefan Monnier]

>> I don't understand enough of the code to have an opinion on it, but the
>> comments describe a behavior which would be wrong: both before-c-f and
>> after-c-f- need to be run for any buffer change, even if it's only an
>> insertion or only a deletion.
> What if there's no change at all, i.e. no deletions and no insertions?

Then you can either run neither of the hooks, or both.

> That was the OP's recipe.

My comment wasn't directly related to the bug-report.

>> > And what would you expect to see in the buffer
>> > modification hooks calls for the recipe of that bug?
>> 
>> Lots of options, but basically: one call to b-c-f with FROM being < then
>> the first position at which a deletion or insertion will take place and
>> TO being after the last such position (e.g. from==to if the change is
>> a single insertion); followed by one call to a-c-f with the
>> same constraints (e.g. from==to if it's a single deletion).
>
> You did read the bug report, didn't you?  Because unless I completely
> misunderstand what you are saying, you are not describing the bug's
> recipe.

Yes, I did.  All I read in the report seems fine, and your commit
message sounds right as well.  But the comments seem to describe an
incorrect behavior.


        Stefan

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 21 Jul 2018 14:44:54 -0400
> 
> >> I don't understand enough of the code to have an opinion on it, but the
> >> comments describe a behavior which would be wrong: both before-c-f and
> >> after-c-f- need to be run for any buffer change, even if it's only an
> >> insertion or only a deletion.
> > What if there's no change at all, i.e. no deletions and no insertions?
> 
> Then you can either run neither of the hooks, or both.

How can we determine whether to run neither or not?

I can easily run both, but is that TRT?  It was you who requested not
to run the hooks on a range that is larger than we can determine by
looking at the results of compareseq.

> > You did read the bug report, didn't you?  Because unless I completely
> > misunderstand what you are saying, you are not describing the bug's
> > recipe.
> 
> Yes, I did.  All I read in the report seems fine, and your commit
> message sounds right as well.  But the comments seem to describe an
> incorrect behavior.

The recipe describes a case where "foo" is replaced by "foo", and the
code in compareseq tells us not to change anything.  There are no
deletions and no insertions.  Do we call the modification hooks in
this case?

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Stefan Monnier]

>> >> I don't understand enough of the code to have an opinion on it, but the
>> >> comments describe a behavior which would be wrong: both before-c-f and
>> >> after-c-f- need to be run for any buffer change, even if it's only an
>> >> insertion or only a deletion.
>> > What if there's no change at all, i.e. no deletions and no insertions?
>> Then you can either run neither of the hooks, or both.
> How can we determine whether to run neither or not?

You can roll a dice.

> I can easily run both, but is that TRT?  It was you who requested not
> to run the hooks on a range that is larger than we can determine by
> looking at the results of compareseq.

Running both would not be a bug: it would be sub-optimal but not
incorrect (just like your original choice of running the hooks over the
whole buffer).
If it's easy to do the better choice is to do nothing.

> The recipe describes a case where "foo" is replaced by "foo", and the
> code in compareseq tells us not to change anything.  There are no
> deletions and no insertions.  Do we call the modification hooks in
> this case?

If we can refrain from calling them, it's better to do that.

But this thread is not about the case described in the bug-report, where
there's been no change.

It's about the text in the two comments you introduced: they say (or at
least can be interpreted to say) that the code may run only one of the
two hooks in some cases.  If that text is a correct description of the
code, it means we have a bug in the code, and if not it means those
comments should be improved.


        Stefan

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > You can roll a dice.

"Dice" is the plural.  The singular is "die".

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 21 Jul 2018 16:46:47 -0400
> Cc: emacs-devel@gnu.org
> 
> It's about the text in the two comments you introduced: they say (or at
> least can be interpreted to say) that the code may run only one of the
> two hooks in some cases.  If that text is a correct description of the
> code, it means we have a bug in the code, and if not it means those
> comments should be improved.

Well, I hope you are happy now, because those comments are gone.

Re: [Emacs-diffs] emacs-26 671dc5a: Fix calls to buffer modification hooks from replace-buffer-contents

[Stefan Monnier]

>> It's about the text in the two comments you introduced: they say (or at
>> least can be interpreted to say) that the code may run only one of the
>> two hooks in some cases.  If that text is a correct description of the
>> code, it means we have a bug in the code, and if not it means those
>> comments should be improved.
> Well, I hope you are happy now, because those comments are gone.

I wasn't unhappy, I was just wondering,


        Stefan