`save-excursion' defeated by `set-buffer'

`save-excursion' defeated by `set-buffer'

[Eli Zaretskii]

With today's CVS, I see this warning in files.el and in gnus-group.el:

  In recover-session-finish:
  files.el:5079:20:Warning: `save-excursion' defeated by `set-buffer'

  In gnus-group-set-mode-line:
  gnus/gnus-group.el:1770:22:Warning: `save-excursion' defeated by `set-buffer'
  gnus/gnus-group.el:1785:39:Warning: `save-excursion' defeated by `set-buffer'

  In gnus-group-quit:
  gnus/gnus-group.el:4483:22:Warning: `save-excursion' defeated by `set-buffer'

  In gnus-group-set-info:
  gnus/gnus-group.el:4564:17:Warning: `save-excursion' defeated by `set-buffer'
  gnus/gnus-group.el:4556:39:Warning: `save-excursion' defeated by `set-buffer'

  In gnus-add-mark:
  gnus/gnus-group.el:4616:35:Warning: `save-excursion' defeated by `set-buffer'

Re: `save-excursion' defeated by `set-buffer'

[Juanma Barranquero]

On Fri, Dec 18, 2009 at 10:20, Eli Zaretskii <eliz@gnu.org> wrote:

> With today's CVS, I see this warning in files.el and in gnus-group.el:

There are 314 such warnings during bootstrap. Stefan said:

"[...] the ones that show up at compile time are not a problem (they
are potential bugs, but they've been around for a while, so there's no
hurry to fix them)."

http://lists.gnu.org/archive/html/emacs-devel/2009-12/msg00239.html

    Juanma

Re: `save-excursion' defeated by `set-buffer'

[Thierry Volpiatto]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Dec 18, 2009 at 10:20, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> With today's CVS, I see this warning in files.el and in gnus-group.el:
>
> There are 314 such warnings during bootstrap. Stefan said:
>
> "[...] the ones that show up at compile time are not a problem (they
> are potential bugs, but they've been around for a while, so there's no
> hurry to fix them)."

However, constructs like:

(save-excursion (set-buffer foo) [.....])

lead to this kind of warnings, and these constructs are still described
in some places in info like here:

(info "(elisp)Creating Buffer-Local")


-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France

Re: `save-excursion' defeated by `set-buffer'

[Roland Winkler]

Me too I've seen these messages with some code I've been using. Yet
I wasn't sure how to interpret them / what to do with them. I looked
into the elisp manual, but I didn't find anything. It would be great
if someone could add a remark to the elisp manual and / or
docstrings about the relation between save-excursion, set-buffer and
with-current-buffer.

Thanks,

Roland

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> Me too I've seen these messages with some code I've been using. Yet
> I wasn't sure how to interpret them / what to do with them. I looked
> into the elisp manual, but I didn't find anything. It would be great
> if someone could add a remark to the elisp manual and / or
> docstrings about the relation between save-excursion, set-buffer and
> with-current-buffer.

save-excursion only saves point in the current buffer, so

   (save-excursion (set-buffer foo) (goto-char (point-min)))

will move point in foo and the point-saving done by save-excursion is
useless.  So either you want to use

   (save-current-buffer (set-buffer foo) (goto-char (point-min)))
aka
   (with-current-buffer foo (goto-char (point-min)))

if moving point in foo is what you wanted, or

   (with-current-buffer foo (save-excursion (goto-char (point-min))))

if you didn't want to move point in foo.


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Me too I've seen these messages with some code I've been using. Yet
>> I wasn't sure how to interpret them / what to do with them. I looked
>> into the elisp manual, but I didn't find anything. It would be great
>> if someone could add a remark to the elisp manual and / or
>> docstrings about the relation between save-excursion, set-buffer and
>> with-current-buffer.
>
> save-excursion only saves point in the current buffer, so
>
>    (save-excursion (set-buffer foo) (goto-char (point-min)))
>
> will move point in foo and the point-saving done by save-excursion is
> useless.

The buffer saving isn't.  The DOC string of save-excursion states:

    save-excursion is a special form in `C source code'.

    (save-excursion &rest BODY)

    Save point, mark, and current buffer; execute BODY; restore those things.
    Executes BODY just like `progn'.
    The values of point, mark and the current buffer are restored
    even in case of abnormal exit (throw or error).
    The state of activation of the mark is also restored.

    This construct does not save `deactivate-mark', and therefore
    functions that change the buffer will still cause deactivation
    of the mark at the end of the command.  To prevent that, bind
    `deactivate-mark' with `let'.

>  So either you want to use
>
>    (save-current-buffer (set-buffer foo) (goto-char (point-min)))
> aka
>    (with-current-buffer foo (goto-char (point-min)))

Says who?  You mean "so either you _could_ use".  But save-excursion
does save the mark in the current buffer, and it does save point and
mark of the current buffer.

Please check the difference of
(with-temp-buffer
  (let ((buf1 (current-buffer)))
     (insert "xxxx")
     (prin1 (point))
     (save-excursion
       (with-temp-buffer
          (with-current-buffer buf1
            (goto-char (point-min)))))
     (prin1 (point))))
and
(with-temp-buffer
  (let ((buf1 (current-buffer)))
     (insert "xxxx")
     (prin1 (point))
     (with-temp-buffer
        (with-current-buffer buf1
          (goto-char (point-min))))
     (prin1 (point))))

You'll see that save-excursion restores point (and mark) even when
temporarily moving into some other buffer.

> if moving point in foo is what you wanted, or
>
>    (with-current-buffer foo (save-excursion (goto-char (point-min))))
>
> if you didn't want to move point in foo.

And what if I wanted to have mark, point, and buffer restored, as the
DOC string of save-excursion states as the purpose of save-excursion
without telling you that the compiler will start nagging you if you use
it for that purpose?

I think the warning is a mistake.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[martin rudalics]

 > I think the warning is a mistake.

I think having the warning in the 23.2 release is a mistake.  Glenn and
others have spent a lot of time to make bootstrapping smooth for the
23.1 release so I was finally able to concentrate on the remaining
issues.  With the present warning bootstrapping has become as cumbersome
as with the pre-23 releases :-(

Why not keep this issue for the 24 release and consider each and every
instance of the problem in separation so we can sort out the concerns
mentioned by David?

martin

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

>> So either you want to use
>> 
>> (save-current-buffer (set-buffer foo) (goto-char (point-min)))
>> aka
>> (with-current-buffer foo (goto-char (point-min)))

> Says who?

I, based on my experience.

> You mean "so either you _could_ use".  But save-excursion
> does save the mark in the current buffer, and it does save point and
> mark of the current buffer.

> Please check the difference of
> (with-temp-buffer
>   (let ((buf1 (current-buffer)))
>      (insert "xxxx")
>      (prin1 (point))
>      (save-excursion
>        (with-temp-buffer
>           (with-current-buffer buf1
>             (goto-char (point-min)))))
>      (prin1 (point))))
> and
> (with-temp-buffer
>   (let ((buf1 (current-buffer)))
>      (insert "xxxx")
>      (prin1 (point))
>      (with-temp-buffer
>         (with-current-buffer buf1
>           (goto-char (point-min))))
>      (prin1 (point))))

> You'll see that save-excursion restores point (and mark) even when
> temporarily moving into some other buffer.

Irrelevant: neither example uses (save-excursion (set-buffer ..) ...).

>> if moving point in foo is what you wanted, or
>> (with-current-buffer foo (save-excursion (goto-char (point-min))))
>> if you didn't want to move point in foo.

> And what if I wanted to have mark, point, and buffer restored, as the

Obviously "if you didn't want to move point in foo" implies that you did
not want "to have mark, point, and buffer restored".


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> So either you want to use
>>> 
>>> (save-current-buffer (set-buffer foo) (goto-char (point-min)))
>>> aka
>>> (with-current-buffer foo (goto-char (point-min)))
>
>> Says who?
>
> I, based on my experience.
>
>> You mean "so either you _could_ use".  But save-excursion
>> does save the mark in the current buffer, and it does save point and
>> mark of the current buffer.
>
>> Please check the difference of
>> (with-temp-buffer
>>   (let ((buf1 (current-buffer)))
>>      (insert "xxxx")
>>      (prin1 (point))
>>      (save-excursion
>>        (with-temp-buffer
>>           (with-current-buffer buf1
>>             (goto-char (point-min)))))
>>      (prin1 (point))))
>> and
>> (with-temp-buffer
>>   (let ((buf1 (current-buffer)))
>>      (insert "xxxx")
>>      (prin1 (point))
>>      (with-temp-buffer
>>         (with-current-buffer buf1
>>           (goto-char (point-min))))
>>      (prin1 (point))))
>
>> You'll see that save-excursion restores point (and mark) even when
>> temporarily moving into some other buffer.
>
> Irrelevant: neither example uses (save-excursion (set-buffer ..) ...).

Don't be disingenuous.  with-temp-buffer uses with-current-buffer, which
is basically (save-current-buffer (set-buffer ...

>>> if moving point in foo is what you wanted, or
>>> (with-current-buffer foo (save-excursion (goto-char (point-min))))
>>> if you didn't want to move point in foo.
>
>> And what if I wanted to have mark, point, and buffer restored, as the
>
> Obviously "if you didn't want to move point in foo" implies that you did
> not want "to have mark, point, and buffer restored".

I don't see the obviousness.  For example, there are recursive editing
operations (like in the debugger) which change the buffer, where the
user can switch around buffers and change point and stuff at will, but
where it is important that upon return from the save-excursion,
current-buffer _and_ point are where they were before.  And the DOC
string of save-excursion says that it will do that, and it does not say
that if you use it for that purpose, the compiler will get into
hysterics.

And even _if_ using save-excursion for saving _both_ buffer and its
point was deprecated (for which I see no good reason whatsoever): as
long as the DOC string of save-excursion does not even _mention_
save-current-buffer, it is not appropriate to throw warnings at the
user.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

>> Irrelevant: neither example uses (save-excursion (set-buffer ..) ...).
> Don't be disingenuous.  with-temp-buffer uses with-current-buffer, which
> is basically (save-current-buffer (set-buffer ...

save-current-buffer != save-excursion

This warning is specificaly aimed at reminding people that the two are
different, so you clearly need to see this warning a few more times
before you start to understand what it's about.

>> Obviously "if you didn't want to move point in foo" implies that you did
>> not want "to have mark, point, and buffer restored".

> I don't see the obviousness.

The code I show *does* move point within the save-excursion, so by
"didn't move point" I meant the same as your "restore point".
Hence the "obviousness".

> And even _if_ using save-excursion for saving _both_ buffer and its
> point was deprecated (for which I see no good reason whatsoever): as
> long as the DOC string of save-excursion does not even _mention_
> save-current-buffer, it is not appropriate to throw warnings at
> the user.

Fair enough, I've just added a note to the docstring.


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> Irrelevant: neither example uses (save-excursion (set-buffer ..) ...).
>> Don't be disingenuous.  with-temp-buffer uses with-current-buffer, which
>> is basically (save-current-buffer (set-buffer ...
>
> save-current-buffer != save-excursion
>
> This warning is specificaly aimed at reminding people that the two are
> different, so you clearly need to see this warning a few more times
> before you start to understand what it's about.

And you clearly need to reread my post to understand what it's about.
If you macroexpand with-temp-buffer and save-current-buffer, my argument
still holds: the warning is wrong, and the macro-expanded code
explicitly uses all those commands which you claim are equivalent in
this situation, with different results.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[Roland Winkler]

On Mon Dec 21 2009 Stefan Monnier wrote:
> save-excursion only saves point in the current buffer, so
> 
>    (save-excursion (set-buffer foo) (goto-char (point-min)))
> 
> will move point in foo and the point-saving done by save-excursion is
> useless.  So either you want to use
> 
>    (save-current-buffer (set-buffer foo) (goto-char (point-min)))
> aka
>    (with-current-buffer foo (goto-char (point-min)))
> 
> if moving point in foo is what you wanted, or
> 
>    (with-current-buffer foo (save-excursion (goto-char (point-min))))
> 
> if you didn't want to move point in foo.

Hi Stefan,

Thanks for the clarification! -- I still believe it would be good if
this was mentioned in the elisp manual as the compiler warning by
itself is somewhat cryptic for non-experts.

Roland

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

>>>> Irrelevant: neither example uses (save-excursion (set-buffer ..) ...).
>>> Don't be disingenuous.  with-temp-buffer uses with-current-buffer, which
>>> is basically (save-current-buffer (set-buffer ...
>> save-current-buffer != save-excursion
>> This warning is specifically aimed at reminding people that the two are
>> different, so you clearly need to see this warning a few more times
>> before you start to understand what it's about.
> And you clearly need to reread my post to understand what it's about.
> If you macroexpand with-temp-buffer and save-current-buffer, my argument

save-current-buffer is not a macro.

> still holds: the warning is wrong, and the macro-expanded code
> explicitly uses all those commands which you claim are equivalent in
> this situation, with different results.

Which commands do I claim are equivalent in which circumstance?

Are you saying that (one of) your code(s) using with-temp-buffer causes
the warning to be output?  That would be a bug, indeed.


        Stefan

RE: `save-excursion' defeated by `set-buffer'

[Drew Adams]

So this thread seems to have petered out. I was hoping for some better
understanding.

I would like to see a clear, logical presentation of what your position is,
Stefan. It's not obvious to me what this is all about.

What David said in the thread made sense to me. I'm willing to learn that you
have a good point, Stefan, but so far I don't understand it.

The Elisp manual still (23.1.91) says, e.g., "you should normally use
`set-buffer' within a `save-current-buffer' or `save-excursion'". And it has
model examples that use (save-excursion... (set-buffer...)...), including where
we explain about changing the current buffer.

See nodes `Cleanups', `Creating Buffer-Local', `Current Buffer', and `Clickable
Text'. And there are other nodes that use `switch-to-buffer' within a
`save-excursion' (which would seem to be a similar issue).

Node `Excursions', which provides the doc for `save-excursion', has no example
using `set-buffer', but neither does it say anything that contradicts the use of
`set-buffer' with `save-excursion'.

The explanation in that node is just what I've always understood wrt
`save-excursion'. It makes it clear that only point and mark of the current
buffer are saved and restored, along with the identity of the current buffer. So
I really don't see what the problem is.

`set-buffer' changes only the buffer. `save-excursion' saves and restores only
the identity of the current buffer and its point and mark - nothing more. What
else is there to know about their use together?

What is the real issue? How does `set-buffer' "defeat" `save-excursion' _in
general_? (This is a general warning.)

Sure, if someone doesn't understand what `save-excursion' does, and mistakenly
thinks, e.g., that it restores point and mark in more than just the original
buffer, then the programmer's intention is likely to be defeated.

But that's not a general problem of using the two together; that's just
misunderstanding what they do. If that's the only "problem" we are warning
programmers about, then I think the warning is misguided.

If that's not what this warning is about, then what is? If your way of looking
at this (which is not yet clear to me) is somehow better, then how about a clear
argument? I, for one, am willing to learn, but so far this warning and your
defense of it have at best only confused me instead of enlightening me.

And if the warning is truly useful, then it should say more clearly what the
problem is that it is warning about. If you can't say that in a message, then
have the message point to an explanation in the manual.

Re: `save-excursion' defeated by `set-buffer'

[Davis Herring]

> save-excursion only saves point in the current buffer, so
>
>    (save-excursion (set-buffer foo) (goto-char (point-min)))
>
> will move point in foo and the point-saving done by save-excursion is
> useless.  So either you want to use

If we know that (eq foo (current-buffer)), we should drop the
`set-buffer'.  If we know that not to be the case, we should use
`with-current-buffer' (or perhaps `save-current-buffer').  But what if it
could but needn't be?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

"Davis Herring" <herring@lanl.gov> writes:

>> save-excursion only saves point in the current buffer, so
>>
>>    (save-excursion (set-buffer foo) (goto-char (point-min)))
>>
>> will move point in foo and the point-saving done by save-excursion is
>> useless.  So either you want to use
>
> If we know that (eq foo (current-buffer)), we should drop the
> `set-buffer'.  If we know that not to be the case, we should use
> `with-current-buffer' (or perhaps `save-current-buffer').  But what if it
> could but needn't be?

And what if we don't have just (goto-char (point-min)), but some code
that might _revisit_ the original buffer (with set-buffer or otherwise)
and _move_ its point?  Then save-excursion _will_ restore the original
point in the buffer at its end which otherwise would remain moved.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> I would like to see a clear, logical presentation of what your position is,
> Stefan.  It's not obvious to me what this is all about.

It's really not about much.  Feel free to completely ignore it.
I've updated the manual (except for the lisp-intro) to eliminate the
more obvious problems.

> The explanation in that node is just what I've always understood wrt
> `save-excursion'.  It makes it clear that only point and mark of the
> current buffer are saved and restored, along with the identity of the
> current buffer.  So I really don't see what the problem is.

The propblem is that even tho it's clear, people tend to forget about
it.  Proof is the dired-mouse-find-file-other-window example code in the
manual which did

   (save-excursion
     ...
     (set-buffer ...)
     (goto-char ...)
     ...)

The problem with that code is that point in current buffer will be moved
depending on whether the buffer we switch to is the same as the
current buffer.  That's unlikely to be what the author intended.
She probably either didn't want to save point (and juse use
save-current-buffer instead), or did want to save point (in which case
she needed save-current-buffer around and an additional save-excursion
inside).

> But that's not a general problem of using the two together; that's just
> misunderstanding what they do. If that's the only "problem" we are warning
> programmers about, then I think the warning is misguided.

The precise behavior of (save-excusrion (set-buffer foo) ...) is
sufficiently odd that I still haven't come upon a single real case where
it was the right thing to do.  Yet, this precise form is (well, was)
found more than a hundred times in the Emacs source code.



        Stefan

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> If we know that (eq foo (current-buffer)), we should drop the
> `set-buffer'.  If we know that not to be the case, we should use
> `with-current-buffer' (or perhaps `save-current-buffer').  But what if it
> could but needn't be?

Let's try it this way:

   If we know that (eq foo 0), we should drop the increment.  If we know
   that not to be the case, we should just use (incf bar foo).  But what
   if it could but needn't be?

I.e. just use with-current-buffer and stop worrying about the case where
it may end up doing nothing.


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

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

>> I would like to see a clear, logical presentation of what your position is,
>> Stefan.  It's not obvious to me what this is all about.
>
> It's really not about much.  Feel free to completely ignore it.
> I've updated the manual (except for the lisp-intro) to eliminate the
> more obvious problems.
>
>> The explanation in that node is just what I've always understood wrt
>> `save-excursion'.  It makes it clear that only point and mark of the
>> current buffer are saved and restored, along with the identity of the
>> current buffer.  So I really don't see what the problem is.
>
> The propblem is that even tho it's clear, people tend to forget about
> it.  Proof is the dired-mouse-find-file-other-window example code in the
> manual which did
>
>    (save-excursion
>      ...
>      (set-buffer ...)
>      (goto-char ...)
>      ...)
>
> The problem with that code is that point in current buffer will be moved
> depending on whether the buffer we switch to is the same as the
> current buffer.

And it will get restored in the current (original) buffer at the end of
the save-excursion.

So it is not the save-excursion that is defeated by set-buffer, but
rather the goto-char which may be defeated by save-excursion.

> That's unlikely to be what the author intended.  She probably either
> didn't want to save point (and juse use save-current-buffer instead),
> or did want to save point (in which case she needed
> save-current-buffer around and an additional save-excursion inside).

Or she wanted to have point and buffer restored at the end of the
save-excursion.

-- 
David Kastrup

RE: `save-excursion' defeated by `set-buffer'

[Drew Adams]

> >    (save-excursion
> >      ...
> >      (set-buffer ...)
> >      (goto-char ...)
> >      ...)
> >
> > The problem with that code is that point in current buffer 
> > will be moved depending on whether the buffer we switch to
> > is the same as the current buffer.
> 
> And it will get restored in the current (original) buffer at 
> the end of the save-excursion.
> 
> So it is not the save-excursion that is defeated by set-buffer, but
> rather the goto-char which may be defeated by save-excursion.

Exactly.

But I think it's not helpful to speak here of anything being "defeated" by
anything. The only thing defeated is the user's possible misconception of what
should happen.

Certainly, `set-buffer' does not undo or inhibit or interfere with the normal
behavior of `save-excursion'; it cannot be said to "defeat" `save-excursion' in
any way.

And the `goto-char' works fine. There is nothing wrong with it, regardless of
which buffer is the target of `set-buffer'. It's simply that `save-excursion'
restores some previous state.

But only after its body is finished. For the duration of the body,
`save-excursion' has no effect. What happens inside a `save-excursion' is not an
issue - `save-excursion' does not change the behavior of code inside it. And
aside from non-local exits (e.g. `throw'), its body has no effect on the
behavior of `save-excursion'. AFAIK.

What's at issue here is only user understanding about `save-excursion', AFAICT.
The problem is apparently that some users could forget or not know that
`save-excursion' restores point and mark in the original buffer (only), along
with restoring the current-buffer identity. 

So what? Some users forget or don't know that (setq a '(b)) doesn't necessarily
create new list structure each time it is evaluated. Some users - including any
of us - forget or don't know lots of things.

This is an education problem. Compiler warnings can sometimes help with
education, but only if they are (a) not too noisy, (b) accurate and precise, and
(c) understandable. I see this warning as not helping but, at best, confusing
users.

If you think that byte-compile is clever enough to determine the places where it
really should warn (certainly not every use of `set-buffer' inside
`save-excursion'), then the message should refer the user to the explanation of
`save-excursion' in the manual. The manual can point out the issue raised here,
if it is indeed a common gotcha.

But I doubt that byte-compile is that clever now or that it even could be made
that clever. IMO the warning should simply be removed.

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> Certainly, `set-buffer' does not undo or inhibit or interfere with the
> normal behavior of `save-excursion'; it cannot be said to "defeat"
> `save-excursion' in any way.

The experience so far is that when replacing

  (save-excursion (set-buffer FOO) ...)
with
  (with-current-buffer FOO ...)

either the behavior is unchanged (i.e. the set-buffer makes the
point-saving part of save-excursion useless), or a bug shows up which
can be fixed with the use of

  (with-current-buffer FOO (save-excursion ...))

I.e. the original code only worked right when the set-buffer was a noop
(which for such code is typically the most common case, obviously,
otherwise the bug would have surfaced earlier).

> IMO the warning should simply be removed.

Show me a single case (in existing code) where
(save-excursion (set-buffer FOO) ...) wouldn't better be written some
other way.


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

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

> Show me a single case (in existing code) where (save-excursion
> (set-buffer FOO) ...) wouldn't better be written some other way.

That does not justify a bogus warning.

Show me a single case where

(setq a ...)
(setq a ...)

wouldn't be better be written some other way (with "better" meaning a
completely subjective measure).

Yet we don't warn about it.  We don't press matters of style lightly.

Or more serious, a single case where
(sort xxx ...) or (nconc xxx ...) outside of a (setq xxx ...) wouldn't
better be written some other way.

We don't warn about those either.

-- 
David Kastrup

RE: `save-excursion' defeated by `set-buffer'

[Drew Adams]

> > Show me a single case (in existing code) where (save-excursion
> > (set-buffer FOO) ...) wouldn't better be written some other way.
> 
> That does not justify a bogus warning.
> Show me a single case where
> 
> (setq a ...) (setq a ...)
> 
> wouldn't be better be written some other way (with "better" meaning a
> completely subjective measure). Yet we don't warn about it.
> We don't press matters of style lightly.
> 
> Or more serious, a single case where
> (sort xxx ...) or (nconc xxx ...) outside of a (setq xxx ...) wouldn't
> better be written some other way.
> We don't warn about those either.

Yes, and besides that argument, and without arguing about the "best" style in
which to code things, just what is inherently wrong or dangerous (meriting a
warning) with code like the following, to do some work in other buffers yet
return to the original buffer AND restore its point and mark?

(save-excursion
  (set-buffer FOO)
  ; Pick up some info in FOO, & perhaps assign it to a var
  (set-buffer BAR)
  ; Calculate something in BAR, & perhaps record it too
  ...
)
; Use the gathered info to do something in the
; original buffer at the saved point.

The Elisp manual says: 

"The `save-excursion' special form is the standard way to switch
buffers or move point within one part of a program and avoid
affecting the rest of the program.  It is used more than 4000
times in the Lisp sources of Emacs."

The _standard_ way to _switch buffers_ and/or move point. Imagine that.

It of course does _not_ switch buffers on its own, which means that it is
standard to use it with some buffer-changing command such as `set-buffer' (or
`with-current-buffer' or ...).

It is not the only way. It is not always the best way. But it is one way. And it
has even been called "the standard" way. Since Day 1.

The manual also says:

"The way to designate a current buffer in a Lisp program is by calling
`set-buffer'."

_The way_. Again, that sounds pretty definitive, but the context tones that down
a bit by mentioning some additional ways. None of those alternatives is
presented as _the_ generally preferred way, however. No preference is indicated.

The gist is that `save-excursion' + `set-buffer' is a not unreasonable way to
switch buffers temporarily to do something without affecting the original
context (current buffer, point, mark). A far cry, at least, from warning users
that such code is somehow abhorrent or inherently unsafe.

No one is arguing that one should always use `(save-excursion
(set-buffer...)...)' in preference to other approaches. It's just that a good
case for issuing a _warning_ for that has not been made. The only arguments made
have been in terms of user misunderstanding or preferred coding style. And those
things are better dealt with clearly, at sufficient length to promote
understanding, in the Elisp manual.

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> We don't warn about those either.

Patches welcome ;-)


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> which to code things, just what is inherently wrong or dangerous (meriting a
> warning) with code like the following, to do some work in other buffers yet
> return to the original buffer AND restore its point and mark?

> (save-excursion
>   (set-buffer FOO)
>   ; Pick up some info in FOO, & perhaps assign it to a var
>   (set-buffer BAR)
>   ; Calculate something in BAR, & perhaps record it too
>   ...
> )

What is dangerous about it is that dependong on which buffer is current
before executing this code, the point-saving will either do something
or nothing.  That's a very delicate semantics, which (as mentioned) I've
never found to be correct (i.e. the only times it works reliably is
when the point-saving part of save-excursion is never useful nor
harmful).

Anyway, we just disagree,


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

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

>> which to code things, just what is inherently wrong or dangerous (meriting a
>> warning) with code like the following, to do some work in other buffers yet
>> return to the original buffer AND restore its point and mark?
>
>> (save-excursion
>>   (set-buffer FOO)
>>   ; Pick up some info in FOO, & perhaps assign it to a var
>>   (set-buffer BAR)
>>   ; Calculate something in BAR, & perhaps record it too
>>   ...
>> )
>
> What is dangerous about it is that dependong on which buffer is current
> before executing this code, the point-saving will either do something
> or nothing.

Wrong.  It will always restore the _original_ buffer and its point on
exit of the save-excursion form.

> That's a very delicate semantics, which (as mentioned) I've never
> found to be correct (i.e. the only times it works reliably is when the
> point-saving part of save-excursion is never useful nor harmful).

As an example, when debugging some operation in buffer FOO, I can use
save-excursion in FOO and then set-buffer to the DEBUG buffer.  If the
user now selects FOO and moves around in it in order to look at its
contents, on exit of save-excursion, FOO's point will get restored.
Which is exactly what is required.

> Anyway, we just disagree,

If no consensus between suitably reasonable and informed persons can be
reached, it is a mistake to emit warnings and penalize a programming
style that can't be shown to be wrong.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[Harald Hanche-Olsen]

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

> [...]
>   (with-current-buffer FOO ...)
> [...]
>   (with-current-buffer FOO (save-excursion ...))

An aside: While reading this discussion I wanted to experiment to
understand the difference between the two forms above. If I execute
the following from a buffer other than *scratch*, the letter A is
inserted four places after point, but point does not move:

(with-current-buffer "*scratch*" (forward-char 4) (insert "A"))

However, the doc string for with-current-buffer says nothing about
restoring point. This is confusing. Also, with-current-buffer expands
to save-current-buffer, whose doc string also says nothing about
restoring point. Again confusing.

- Harald

RE: `save-excursion' defeated by `set-buffer'

[Drew Adams]

> Anyway, we just disagree

What about others on the list? Can we get some more input on this? 

Others: Do you appreciate this warning or not? Why? No opinion? Do you
understand why you should not use `set-buffer' with `save-excursion'? If you
like the warning, is its wording and meaning clear enough? Suggestions?

Beyond this list, what about polling the users?

(Have we had any user polls since Richard passed the torch?)

Re: `save-excursion' defeated by `set-buffer'

[martin rudalics]

 > An aside: While reading this discussion I wanted to experiment to
 > understand the difference between the two forms above. If I execute
 > the following from a buffer other than *scratch*, the letter A is
 > inserted four places after point, but point does not move:
 >
 > (with-current-buffer "*scratch*" (forward-char 4) (insert "A"))
 >
 > However, the doc string for with-current-buffer says nothing about
 > restoring point. This is confusing. Also, with-current-buffer expands
 > to save-current-buffer, whose doc string also says nothing about
 > restoring point. Again confusing.

The form above _does_ move `point' unless that's inhibited by other
(usually text) properties.  If *scratch* is displayed in some window,
`point' moves as well but the visible effect is overridden by the fact
that `window-point' in any window showing *scratch* does not move (wrt
to the surrounding text).  So I suppose the behavior you report is a
behavior you observed with *scratch* displayed in some window while you
executed that form.

martin

RE: `save-excursion' defeated by `set-buffer'

[Drew Adams]

> A is inserted four places after point, but point does not move:
> (with-current-buffer "*scratch*" (forward-char 4) (insert "A"))
 
Actually, I think that `point' does move. What doesn't move is `window-point'.
Which is the correct behavior.

Try `M-x foo' (a) with *scratch* displayed and (b) without *scratch* displayed:

(defun foo ()
  (interactive)
  (with-current-buffer "*scratch*"
    (bar) (forward-char 4)
    (bar) (insert "A")
    (bar)))

(defun bar ()
  (message
   "%S, %S, %S"
   (current-buffer)
   (point)
   (and (get-buffer-window "*scratch*" 0)
        (window-point
         (get-buffer-window "*scratch*" 0)))))

Starting with point at 1, you see this:

(a) *scratch* displayed
#<buffer *scratch*>, 1, 1
#<buffer *scratch*>, 5, 1
#<buffer *scratch*>, 6, 1

(b) *scratch* not displayed
#<buffer *scratch*>, 1, nil
#<buffer *scratch*>, 5, nil
#<buffer *scratch*>, 6, nil

Re: `save-excursion' defeated by `set-buffer'

[Harald Hanche-Olsen]

Ah, I had utterly missed the existence of window-point - though
something like it obviously must exist, for multiple windows on the
same buffer to work right. Thanks for the explanation. You can return
to the original topic of discussion now, and I'll go back to lurking.
Sorry about the noise.

- Harald

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

> As an example, when debugging some operation in buffer FOO, I can use
> save-excursion in FOO and then set-buffer to the DEBUG buffer.  If the

You can still do it just as well.


        Stefan

Re: `save-excursion' defeated by `set-buffer'

[David Kastrup]

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

>> As an example, when debugging some operation in buffer FOO, I can use
>> save-excursion in FOO and then set-buffer to the DEBUG buffer.  If the
>
> You can still do it just as well.

And get a warning when doing so.  Where is the point in warnings that
are both inaccurate ("save-excursion defeated by set-buffer" is
nonsensical, since save-excursion still perfectly well does its
documented job of restoring _original_ buffer, point, mark) as well as
inapplicable?

If you say that you find most (save-excursion (set-buffer combinations
in the Emacs code base a bad idea, the solution is using grep and fixing
all those occurences where you think they are an error.

Emitting a warning changes no existing code, annoys people, and flags
code as problematic that isn't.  Possibly code that is written to be
compatible with versions of Emacs without save-current-buffer.

-- 
David Kastrup

Re: `save-excursion' defeated by `set-buffer'

[martin rudalics]

 > If you say that you find most (save-excursion (set-buffer combinations
 > in the Emacs code base a bad idea, the solution is using grep and fixing
 > all those occurences where you think they are an error.

Admittedly, there are too many occurrences of this idiom to be fixed by
a single person in reasonable time.  I suppose Stefan, when fixing a bug
incorrectly using that idiom, decided that the respective authors should
care about the remaining instances.  As a matter of fact, the majority
of them refuses to do so and the rest of us continue to live with the
annoying consequences.

IMHO, the best way to address this problem would have been to provide a
separate routine (which should be also able to catch occurrences like

       (save-excursion
  	(if buffer
  	    (set-buffer buffer))

in xml.el), get back to the various authors and ask them to fix these
issues.

martin

Re: `save-excursion' defeated by `set-buffer'

[Stefan Monnier]

>> If you say that you find most (save-excursion (set-buffer combinations
>> in the Emacs code base a bad idea, the solution is using grep and fixing
>> all those occurences where you think they are an error.
> Admittedly, there are too many occurrences of this idiom to be fixed by
> a single person in reasonable time.

I do have them all fixed here (only for files bundles with Emacs, obviously).

> I suppose Stefan, when fixing a bug incorrectly using that idiom,
> decided that the respective authors should care about the
> remaining instances.

No, as you may have noticed I installed a whole bunch of little patches
to most of lisp/**/*.el to fix those issues.  The remaining ones are in
lisp/gnus.


        Stefan