with-current-buffer "cleanup" bug

with-current-buffer "cleanup" bug

[Chong Yidong]

Hi Stefan,

I just reverted your change to help-mode.el, because it led to a bug:
`C-h f describe-minor-mode', and any other help command that creates a
help buffer larger than its window size, left point at the end of the
buffer.

If you're wholesale changing `save-excursion + set-buffer' to
`with-current-buffer', this is wrong.  The `with-current-buffer' macro
does not save point (or mark), so this can lead to code behavior change.
You need to fix this, or we're likely see a long slow trickle of related
bugs in the coming weeks and months.


*** help-mode.el.~1.68~	2009-11-14 10:40:18.000000000 -0500
--- help-mode.el.~1.69.~	2009-11-14 10:30:40.000000000 -0500
***************
*** 413,420 ****
  help buffers.  Variable `help-back-label' specifies the text for
  that."
    (interactive "b")
!   (save-excursion
!     (set-buffer (or buffer (current-buffer)))
      (goto-char (point-min))
      ;; Skip the header-type info, though it might be useful to parse
      ;; it at some stage (e.g. "function in `library'").
--- 413,419 ----
  help buffers.  Variable `help-back-label' specifies the text for
  that."
    (interactive "b")
!   (with-current-buffer (or buffer (current-buffer))
      (goto-char (point-min))
      ;; Skip the header-type info, though it might be useful to parse
      ;; it at some stage (e.g. "function in `library'").

Re: with-current-buffer "cleanup" bug

[Stefan Monnier]

> If you're wholesale changing `save-excursion + set-buffer' to
> `with-current-buffer', this is wrong.

The change may be wrong, but the original code is wrong too.  Which is
why I'm happily applying the change: when it uncovers a bug, the right
fix is almost always to use some other code because the problem can
appear even with the original code.

> The `with-current-buffer' macro
> does not save point (or mark), so this can lead to code behavior change.
> You need to fix this, or we're likely see a long slow trickle of related
> bugs in the coming weeks and months.

Yes, I'm waiting for those bugs indeed,


        Stefan

Re: with-current-buffer "cleanup" bug

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The `with-current-buffer' macro does not save point (or mark), so
>> this can lead to code behavior change.  You need to fix this, or
>> we're likely see a long slow trickle of related bugs in the coming
>> weeks and months.
>
> Yes, I'm waiting for those bugs indeed,

If you're not going to test your changes, how about delaying these
patches till after 23.2?

Re: with-current-buffer "cleanup" bug

[Stefan Monnier]

>>> The `with-current-buffer' macro does not save point (or mark), so
>>> this can lead to code behavior change.  You need to fix this, or
>>> we're likely see a long slow trickle of related bugs in the coming
>>> weeks and months.
>> Yes, I'm waiting for those bugs indeed,
> If you're not going to test your changes, how about delaying these
> patches till after 23.2?

I don't think it's necessary.  The 23.2 pretest will offer plenty of time.


        Stefan

Re: with-current-buffer "cleanup" bug

[Stefan Monnier]

> I just reverted your change to help-mode.el, because it led to a bug:

BTW, any such revert should come with a comment explaining the problem to
avoid going through the same back&forth again.


        Stefan

Re: with-current-buffer "cleanup" bug

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Yes, I'm waiting for those bugs indeed,
>> If you're not going to test your changes, how about delaying these
>> patches till after 23.2?
>
> I don't think it's necessary.  The 23.2 pretest will offer plenty of
> time.

At the least, please refrain from making any more of these silly
"cleanups".  They have negligible upside and considerable downside.