`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

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

[Andreas Röhler]

Hi,

`byte-compile-file' sends a warning

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

with resp. to such a form

  (save-excursion
    (set-buffer blub)
    ...
    )

As the docu of save-excursion says:

Save point, mark, and current buffer; execute BODY; restore those things

would expect previous buffer restored, don't understand
that warning.

Any help?
Thanks

Andreas

--
https://code.launchpad.net/~a-roehler/python-mode/python-mode-components
https://code.launchpad.net/s-x-emacs-werkstatt/

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

[Uday Reddy]

On 3/2/2011 5:12 PM, Andreas Röhler wrote:
>
> Hi,
>
> `byte-compile-file' sends a warning
>
> `save-excursion' defeated by `set-buffer'
>
> with resp. to such a form
>
> (save-excursion
> (set-buffer blub)
> ...
> )

If you change `save-excursion' to `save-current-buffer', the warning 
will go away.  Whether that is the right thing to do or not depends on 
your code.

Cheers,
Uday

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

[Le Wang]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Thu, Mar 3, 2011 at 1:12 AM, Andreas Röhler <
andreas.roehler@easy-emacs.de> wrote:

>
> Hi,
>
> `byte-compile-file' sends a warning
>
> `save-excursion' defeated by `set-buffer'
>

Either the compiler warning or the manual is wrong (
http://www.gnu.org/software/emacs/elisp/html_node/Current-Buffer.html):

Therefore, you should normally use set-buffer within a save-current-buffer
 or *save-excursion* (see
Excursions<http://www.gnu.org/software/emacs/elisp/html_node/Excursions.html#Excursions>)
form that will restore the current buffer when your function is done. Here,
as an example, is a simplified version of the command append-to-buffer:


-- 
Le

[-- Attachment #2: Type: text/html, Size: 2445 bytes --]

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

[Stefan Monnier]

> `save-excursion' defeated by `set-buffer'
[...]
>  (save-excursion
>    (set-buffer blub)
>    ...
>    )

> Save point, mark, and current buffer; execute BODY; restore those things

> would expect previous buffer restored, don't understand
> that warning.

The difference between save-excursion and save-current-buffer is that
the first doesn't just save&restore the current buffer but also "point &
mark".  But if you do `set-buffer' right after save-excursion then most
likely you will change neither point nor mark in the original buffer, so
the extra work performed by save-excursion compared to
save-current-buffer will be useless.
Now that's just a waste of resources but is otherwise harmless.

Unless of course `blub' is already the current buffer to start with.

I.e. whether point movement in "..." is undone by save-excursion will
depend dynamically upon whether the current buffer happens to be `blub',
which leads to subtle bugs.  Hence the warning.


        Stefan

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

[Andreas Röhler]

Am 05.03.2011 05:28, schrieb Stefan Monnier:
>> `save-excursion' defeated by `set-buffer'
> [...]
>>   (save-excursion
>>     (set-buffer blub)
>>     ...
>>     )
>
>> Save point, mark, and current buffer; execute BODY; restore those things
>
>> would expect previous buffer restored, don't understand
>> that warning.
>
> The difference between save-excursion and save-current-buffer is that
> the first doesn't just save&restore the current buffer but also "point&
> mark".  But if you do `set-buffer' right after save-excursion then most
> likely you will change neither point nor mark in the original buffer, so
> the extra work performed by save-excursion compared to
> save-current-buffer will be useless.
> Now that's just a waste of resources but is otherwise harmless.
>
> Unless of course `blub' is already the current buffer to start with.
>
> I.e. whether point movement in "..." is undone by save-excursion will
> depend dynamically upon whether the current buffer happens to be `blub',
> which leads to subtle bugs.  Hence the warning.
>
>
>          Stefan
>

Thanks, this warning goes away if body is wrapped with a let:


(save-excursion
   (let ()
     (set-buffer blub)
       ...
       ))

(?)

Andreas

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

[David Kastrup]

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

>> `save-excursion' defeated by `set-buffer'
> [...]
>>  (save-excursion
>>    (set-buffer blub)
>>    ...
>>    )
>
>> Save point, mark, and current buffer; execute BODY; restore those things
>
>> would expect previous buffer restored, don't understand
>> that warning.
>
> The difference between save-excursion and save-current-buffer is that
> the first doesn't just save&restore the current buffer but also "point &
> mark".  But if you do `set-buffer' right after save-excursion then most
> likely you will change neither point nor mark in the original buffer,

"most likely".  So `save-excursion' is most likely defeated by
`set-buffer'.

> so the extra work performed by save-excursion compared to
> save-current-buffer will be useless.  Now that's just a waste of
> resources but is otherwise harmless.
>
> Unless of course `blub' is already the current buffer to start with.
>
> I.e. whether point movement in "..." is undone by save-excursion will
> depend dynamically upon whether the current buffer happens to be `blub',
> which leads to subtle bugs.  Hence the warning.

So the warning tells us "`save-excursion' defeated by `set-buffer'" in
order to warn us against the possibility that `save-excursion' may
actually _not_ defeated by `set-buffer'.

Small wonder nobody understands what the warning is supposed to be for.

-- 
David Kastrup

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

[Leo]

On 2011-03-11 03:57 +0800, Andreas Röhler wrote:
> (save-excursion
>   (let ()
>     (set-buffer blub)
>       ...
>       ))

Doesn't solve the problem the warning tries to get attention to. The
byte-compiler isn't very sophisticated.

Leo

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

[Stefan Monnier]

>> The difference between save-excursion and save-current-buffer is that
>> the first doesn't just save&restore the current buffer but also "point&
>> mark".  But if you do `set-buffer' right after save-excursion then most
>> likely you will change neither point nor mark in the original buffer, so
>> the extra work performed by save-excursion compared to
>> save-current-buffer will be useless.
>> Now that's just a waste of resources but is otherwise harmless.
>> 
>> Unless of course `blub' is already the current buffer to start with.
>> 
>> I.e. whether point movement in "..." is undone by save-excursion will
>> depend dynamically upon whether the current buffer happens to be `blub',
>> which leads to subtle bugs.  Hence the warning.

> Thanks, this warning goes away if body is wrapped with a let:

> (save-excursion
>   (let ()
>     (set-buffer blub)
>       ...
>       ))

I see you apparently didn't understand much of what I wrote :-(


        Stefan

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

[Stefan Monnier]

> So the warning tells us "`save-excursion' defeated by `set-buffer'" in
> order to warn us against the possibility that `save-excursion' may
> actually _not_ defeated by `set-buffer'.
> Small wonder nobody understands what the warning is supposed to be for.

Of course we welcome suggestions for better wording.


        Stefan

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

[Andreas Röhler]

Am 11.03.2011 02:28, schrieb Stefan Monnier:
>>> The difference between save-excursion and save-current-buffer is that
>>> the first doesn't just save&restore the current buffer but also "point&
>>> mark".  But if you do `set-buffer' right after save-excursion then most
>>> likely you will change neither point nor mark in the original buffer, so
>>> the extra work performed by save-excursion compared to
>>> save-current-buffer will be useless.
>>> Now that's just a waste of resources but is otherwise harmless.
>>>
>>> Unless of course `blub' is already the current buffer to start with.
>>>
>>> I.e. whether point movement in "..." is undone by save-excursion will
>>> depend dynamically upon whether the current buffer happens to be `blub',
>>> which leads to subtle bugs.  Hence the warning.
>
>> Thanks, this warning goes away if body is wrapped with a let:
>
>> (save-excursion
>>    (let ()
>>      (set-buffer blub)
>>        ...
>>        ))
>
> I see you apparently didn't understand much of what I wrote :-(
>
>
>          Stefan
>

Maybe let's put the question another way:

is there an example, where save-excursion will fail, ie not restore the 
buffer due to a set-buffer afterwards?

Thanks

Andreas

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

[David Kastrup]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> Maybe let's put the question another way:
>
> is there an example, where save-excursion will fail, ie not restore
> the buffer due to a set-buffer afterwards?

No.  It will restore buffer, point and mark, in that order.  It is never
defeated.  What _may_ be defeated is an excursion happening within the
scope of `set-buffer' because restoring buffer will in some instances
restore to the same buffer before restoring point and mark.

-- 
David Kastrup

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

[Andreas Röhler]

Am 11.03.2011 10:54, schrieb David Kastrup:
> Andreas Röhler<andreas.roehler@easy-emacs.de>  writes:
>
>> Maybe let's put the question another way:
>>
>> is there an example, where save-excursion will fail, ie not restore
>> the buffer due to a set-buffer afterwards?
>
> No.  It will restore buffer, point and mark, in that order.  It is never
> defeated.  What _may_ be defeated is an excursion happening within the
> scope of `set-buffer' because restoring buffer will in some instances
> restore to the same buffer before restoring point and mark.
>

Hmm, isn't it just the task of save-excursion to defeat an inner set-buffer?

A legal defeat so to say...

Or with other words: save-excursion must not restore if no danger buffer 
being set otherwise meanwhile.

So the warning is just nonsense?

Andreas

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

[David Kastrup]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> Am 11.03.2011 10:54, schrieb David Kastrup:
>> Andreas Röhler<andreas.roehler@easy-emacs.de>  writes:
>>
>>> Maybe let's put the question another way:
>>>
>>> is there an example, where save-excursion will fail, ie not restore
>>> the buffer due to a set-buffer afterwards?
>>
>> No.  It will restore buffer, point and mark, in that order.  It is never
>> defeated.  What _may_ be defeated is an excursion happening within the
>> scope of `set-buffer' because restoring buffer will in some instances
>> restore to the same buffer before restoring point and mark.
>>
>
> Hmm, isn't it just the task of save-excursion to defeat an inner
> set-buffer?

It will always defeat the set-buffer (unless they agree in the first
place), that is not the problem.  The problem is that it may also defeat
any excursion (cursor and mark movements) done inside of the buffer
visited with set-buffer (or made while temporarily switching buffer
again within that construct).  When it does _not_ defeat set-buffer, it
will defeat excursions made after set-buffer.

> Or with other words: save-excursion must not restore if no danger
> buffer being set otherwise meanwhile.
>
> So the warning is just nonsense?

There is a difference between "wrong" and "nonsense".  I would classify
the warning message as simply wrong, and the phrase you write after "Or
with other words:" as nonsense.

-- 
David Kastrup

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

[Stefan Monnier]

> is there an example, where save-excursion will fail, ie not restore the
> buffer due to a set-buffer afterwards?

`save-excursion' is not the form to use to save the current buffer: that's
what save-current-buffer is for.  What `save-excursion' does is preserve
point (and mark), and some people tend to think of it as "undoes all
point movement", which is sadly only true for the current buffer but not
all buffers.

So (save-excursion (goto-char BAR)) is pretty much a no-op.

But (save-excursion (set-buffer FOO) (goto-char BAR)) is either:
- the same as (save-excursion (goto-char BAR)), in case FOO is already current.
- an inefficient form of (with-current-buffer FOO (goto-char BAR)).
I still haven't found any code out there where this behavior is what
is actually wanted and expected by the programmer.

In more than 90% of the cases, the intended meaning is
(with-current-buffer FOO (goto-char BAR)) and the behavior if FOO is
current (to additionally preserve point) is harmless, so the warning
simply points out an inefficiency.

In the remaining cases, FOO is almost always current, but when it's not
the resulting behavior is a bug.  I.e. the intended code is
(with-current-buffer FOO (save-excursion (goto-char BAR))).


        Stefan

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

[Uday Reddy]

On 3/11/2011 8:52 AM, Andreas Röhler wrote:
>
> Maybe let's put the question another way:
>
> is there an example, where save-excursion will fail, ie not restore the
> buffer due to a set-buffer afterwards?

That is not the right question to ask, in my opinion.  save-excursion 
will restore the buffer.  But save-current-buffer, which I recommended 
right at the outset of this discussion, will also restore the buffer. 
So, you should be asking the question, "why am I using save-excursion 
instead of save-current-buffer?"  After all, if you are changing 
buffers, then you want to restore the current-buffer in the end.  You 
don't need to restore the point and mark.

Despite the obviousness of save-current-buffer/set-buffer pattern, there 
is a lot of code out there that uses the save-excursion/set-buffer 
pattern.  Why that is, I don't know.  But, here is the problem that can 
arise with this misused pattern.  Suppose you are in buffer A and you run:

    (save-excursion
       (set-buffer B)
       (goto-char x)
        ..... lots of deep function calls, say
        (....
           (set-buffer A)
           (goto-char y)
         ...))))

Two problems here:

- B's point has moved to x.  Is that supposed to be permanent?  It is 
not clear.  The save-excursion may seem to indicate that we want to 
preserve its point.  But, it is only A's point that is being preserved. 
  B's point is not.

- A's point is being moved to y deep inside the code.  Is that supposed 
to be permanent?  Again, it is not clear.  That movement is being 
cancelled by the outer save-excursion.  But if the intent is to restore 
A's point after the temporary movement to y, shouldn't the 
save-excursion be close to the (goto-char y) statement?

If tomorrow, you happen to run this code in buffer C, you will find that 
A's point is suddenly moving.  (The outer save-excursion will restore 
C's point instead of A's point.)  If your code is sufficiently 
complicated, it will probably take quite some digging to find the problem.

So the right programming style is to use

     (save-current-buffer
         (set-buffer B)
         ...)

or

     (save-excursion
         (goto-char y)
         ....)

But

    (save-excursion
         (set-buffer B)
         ...)

is neither here nor there.  Hence, the compiler warning.

Hope this helps.

Cheers,
Uday

PS: By the way, I maintain the mail reader VM, which has tons and tons 
of the bad save-excursion/set-buffer pattern.  But changing those 
save-excursion's to save-current-buffer's is not easy.  There could be 
unprotected point movements buried deep inside which happen to work 
coincidentally because they are buried under a stack of save-excursion 
forms.  So, it is best to use the right programming style from the 
beginning to save unnecessary head aches down the road.

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 11 Mar 2011 10:52:14 -0500
> 
> So (save-excursion (goto-char BAR)) is pretty much a no-op.
> 
> But (save-excursion (set-buffer FOO) (goto-char BAR)) is either:
> - the same as (save-excursion (goto-char BAR)), in case FOO is already current.
> - an inefficient form of (with-current-buffer FOO (goto-char BAR)).
> I still haven't found any code out there where this behavior is what
> is actually wanted and expected by the programmer.
> 
> In more than 90% of the cases, the intended meaning is
> (with-current-buffer FOO (goto-char BAR)) and the behavior if FOO is
> current (to additionally preserve point) is harmless, so the warning
> simply points out an inefficiency.
> 
> In the remaining cases, FOO is almost always current, but when it's not
> the resulting behavior is a bug.  I.e. the intended code is
> (with-current-buffer FOO (save-excursion (goto-char BAR))).

Then how about changing the text of the warning to something like

  Warning: `save-excursion' will not preserve point in the other buffer
  set by `set-buffer'

?

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

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

> Then how about changing the text of the warning to something like
>
>   Warning: `save-excursion' will not preserve point in the other buffer
>   set by `set-buffer'

Is there a particular reason that nobody is interested in letting the
warning be about the case that is supposed to be problematic, rather
than about a number of _intended_ parts of the behavior?

The problem seems more like

Warning: `save-excursion' will preserve point and mark in the current
         buffer even if set-buffer does not actually change buffers.

So why do people want to warn about non-problems and/or utter nonsense
(like the current "save-excursion defeated by set-buffer") all the time?

What is wrong with warning about the problem itself?

-- 
David Kastrup

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

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Date: Sat, 12 Mar 2011 10:34:45 +0100
> 
> >   Warning: `save-excursion' will not preserve point in the other buffer
> >   set by `set-buffer'
> 
> Is there a particular reason that nobody is interested in letting the
> warning be about the case that is supposed to be problematic

Huh?  That's what I tried to express in the above text.  Isn't that
_precisely_ the problematic case, when the other buffer is not the
current and the body of save-excursion moves point and/or mark there?
If that's not the problematic case, please cough up why not, rather
than asking rhetorical questions.

> The problem seems more like
> 
> Warning: `save-excursion' will preserve point and mark in the current
>          buffer even if set-buffer does not actually change buffers.

Your "warning" describes what `save-excursion' is supposed to do, at
least according to the ELisp manual:

       The `save-excursion' special form saves the identity of the current
       buffer and the values of point and the mark in it, evaluates BODY,
       and finally restores the buffer and its saved values of point and
       the mark.

So how could warning about the normal operation be TRT?

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

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: David Kastrup <dak@gnu.org>
>> Date: Sat, 12 Mar 2011 10:34:45 +0100
>> 
>> >   Warning: `save-excursion' will not preserve point in the other buffer
>> >   set by `set-buffer'
>> 
>> Is there a particular reason that nobody is interested in letting the
>> warning be about the case that is supposed to be problematic
>
> Huh?  That's what I tried to express in the above text.  Isn't that
> _precisely_ the problematic case, when the other buffer is not the
> current and the body of save-excursion moves point and/or mark there?

No, it isn't.  The likelilood that somebody uses `set-buffer' when he
expects it not to change the current buffer is not exactly large.

> If that's not the problematic case, please cough up why not, rather
> than asking rhetorical questions.
>
>> The problem seems more like
>> 
>> Warning: `save-excursion' will preserve point and mark in the current
>>          buffer even if set-buffer does not actually change buffers.
>
> Your "warning" describes what `save-excursion' is supposed to do, at
> least according to the ELisp manual:
>
>        The `save-excursion' special form saves the identity of the current
>        buffer and the values of point and the mark in it, evaluates BODY,
>        and finally restores the buffer and its saved values of point and
>        the mark.
>
> So how could warning about the normal operation be TRT?

Very funny.  Of course the warning will be about a situation where every
function works according to its specifications.  Otherwise, it would be
reason for either a bug fix or an error message rather than a warning.

Anyway, I suggest you get yourself up to speed by reading a few messages
from Stefan Monnier about his rationale for introducing this warning.

-- 
David Kastrup

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

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Date: Sat, 12 Mar 2011 11:42:12 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: David Kastrup <dak@gnu.org>
> >> Date: Sat, 12 Mar 2011 10:34:45 +0100
> >> 
> >> >   Warning: `save-excursion' will not preserve point in the other buffer
> >> >   set by `set-buffer'
> >> 
> >> Is there a particular reason that nobody is interested in letting the
> >> warning be about the case that is supposed to be problematic
> >
> > Huh?  That's what I tried to express in the above text.  Isn't that
> > _precisely_ the problematic case, when the other buffer is not the
> > current and the body of save-excursion moves point and/or mark there?
> 
> No, it isn't.  The likelilood that somebody uses `set-buffer' when he
> expects it not to change the current buffer is not exactly large.

And that is exactly the situation that my suggested warning is about.

> >> Warning: `save-excursion' will preserve point and mark in the current
> >>          buffer even if set-buffer does not actually change buffers.
> >
> > Your "warning" describes what `save-excursion' is supposed to do, at
> > least according to the ELisp manual:
> >
> >        The `save-excursion' special form saves the identity of the current
> >        buffer and the values of point and the mark in it, evaluates BODY,
> >        and finally restores the buffer and its saved values of point and
> >        the mark.
> >
> > So how could warning about the normal operation be TRT?
> 
> Very funny.  Of course the warning will be about a situation where every
> function works according to its specifications.

Very funny.  The warning should be about a situation where what the
function is supposed to do does not match what the programmer might
expect from the function, judging by the code the programmer actually
wrote.

Perhaps you could explain how saying that `save-excursion' _will_
preserve point and mark is supposed to help the programmer understand
her possible mistake, which is that `save-excursion' might _not_
preserve them.  Let's see:

  J. R. Hacker: Let's use `save-excursion' to preserve point and
                mark in both this and the other buffer.  [Hacks away.]
  Emacs: Warning: `save-excursion' will preserve point and mark in the
                current buffer even if set-buffer does not actually
                change buffers.
  J. R. Hacker: ?? Isn't that what I wanted, to preserve point and
                mark?  Why the warning???  Stupid Emacs!
		[Leaves the code as is.]

> Anyway, I suggest you get yourself up to speed by reading a few messages
> from Stefan Monnier about his rationale for introducing this warning.

Actually, I did.  Perhaps so should you.  Or, if you see some flaw in
my suggested text, how about pointing out when and how it could lead
J. R. Hacker to the wrong conclusion?

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

[Uday Reddy]

On 3/12/2011 10:42 AM, David Kastrup wrote:

> No, it isn't.  The likelilood that somebody uses `set-buffer' when he
> expects it not to change the current buffer is not exactly large.

Agreed.  But that is not the only case where the problems occur.  As 
explained in my message yesterday, there could be unprotected point 
movements deep inside function calls, which are unintentionally caught 
by the save-excursion at the outer level.  Ideally, those point 
movements should be protected where they occur, not by an unrelated an 
save-excursion sitting somewhere else.

Frankly, I think the warning message is quite fine.  save-excursion is 
trying to preserve the point (and mark), but only for the 
current-buffer.  set-buffer is changing the current-buffer and, so, the 
preservation of the point in the current-buffer is useless.

If one wants a completely plain warning message, it could be:

   "save-excursion has the effect of save-current-buffer"

It doesn't say very much, but nobody will presumably argue about it.

Cheers,
Uday

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

[David Kastrup]

Uday Reddy <uDOTsDOTreddy@cs.bham.ac.uk> writes:

> On 3/12/2011 10:42 AM, David Kastrup wrote:
>
>> No, it isn't.  The likelilood that somebody uses `set-buffer' when he
>> expects it not to change the current buffer is not exactly large.
>
> Agreed.  But that is not the only case where the problems occur.  As
> explained in my message yesterday, there could be unprotected point
> movements deep inside function calls, which are unintentionally caught
> by the save-excursion at the outer level.  Ideally, those point
> movements should be protected where they occur, not by an unrelated an
> save-excursion sitting somewhere else.
>
> Frankly, I think the warning message is quite fine.  save-excursion is
> trying to preserve the point (and mark), but only for the
> current-buffer.  set-buffer is changing the current-buffer and, so,
> the preservation of the point in the current-buffer is useless.

There is little reason to warn about "useless" code.

> If one wants a completely plain warning message, it could be:
>
>   "save-excursion has the effect of save-current-buffer"
>
> It doesn't say very much, but nobody will presumably argue about it.

The most plausible theory I have for this thread is that you are
collectively trying to pull my leg.

If you give the user that warning, he'll say "great, just like I wanted
it to do".

Why warn the user that "save-excursion" is doing what he wants it to do,
when the actual problem is that it may do more, namely reverting an
excursion happening unintendedly in the original buffer?

-- 
David Kastrup

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

[Andreas Röhler]

Am 12.03.2011 09:59, schrieb Eli Zaretskii:
>> From: Stefan Monnier<monnier@iro.umontreal.ca>
>> Date: Fri, 11 Mar 2011 10:52:14 -0500
>>
>> So (save-excursion (goto-char BAR)) is pretty much a no-op.
>>
>> But (save-excursion (set-buffer FOO) (goto-char BAR)) is either:
>> - the same as (save-excursion (goto-char BAR)), in case FOO is already current.
>> - an inefficient form of (with-current-buffer FOO (goto-char BAR)).
>> I still haven't found any code out there where this behavior is what
>> is actually wanted and expected by the programmer.
>>
>> In more than 90% of the cases, the intended meaning is
>> (with-current-buffer FOO (goto-char BAR)) and the behavior if FOO is
>> current (to additionally preserve point) is harmless, so the warning
>> simply points out an inefficiency.
>>
>> In the remaining cases, FOO is almost always current, but when it's not
>> the resulting behavior is a bug.  I.e. the intended code is
>> (with-current-buffer FOO (save-excursion (goto-char BAR))).
>
> Then how about changing the text of the warning to something like
>
>    Warning: `save-excursion' will not preserve point in the other buffer
>    set by `set-buffer'
>
> ?
>
>



Seems all what's neccessary to know about save-excursion already is 
expressed in it's docstring.

If people don't want the buffer restored alongside with point and mark, 
they should not use this form.

As I used it, compiler may assume I wanted that. No reason for a warning 
if used as provided.

OTOH we need compiler warnings for serious things.

So my suggestion is simply to drop that warning.

Thanks all

Andreas

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

[Drew Adams]

> Seems all what's neccessary to know about save-excursion already is 
> expressed in it's docstring.
> 
> If people don't want the buffer restored alongside with point 
> and mark, they should not use this form.
> 
> As I used it, compiler may assume I wanted that. No reason 
> for a warning if used as provided.
> 
> OTOH we need compiler warnings for serious things.
> 
> So my suggestion is simply to drop that warning.

1+

But this has already been expressed (by several people), and rejected, in
various discussions in emacs-devel@gnu.org.

IMO, this warning has produced _far_ more confusion than it has eliminated.  And
that will no doubt continue to be the case going forward.

The mere fact that Eli Z and David K have very different interpretations of the
potential problem(s) that this warning hopes to avoid should be demonstration
enough that the warning is not helpful.  Emacs development contributors (and
users) do not come much more experienced than Eli and David (modulo RMS).

And no, the message should not simply be improved/clarified.  It should be
eliminated, just as Andreas suggested.

FWIW, I also agree with Andreas that a "warning" is for something serious.  A
warning is not the same thing as in informative message.  A warning _warns_ you
about potential danger/damage.  Alarmism eventually results in the Chicken
Little effect (aka Boy Cries "Wolf!").

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

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Date: Sat, 12 Mar 2011 11:56:23 -0800
> Cc: 
> 
> The mere fact that Eli Z and David K have very different interpretations of the
> potential problem(s)

I didn't interpret anything.  I just read what Stefan wrote, and
perhaps misunderstood him (though I don't think so).

Anyway, this entire discussion is misplaced.  The right place to
discuss this warning is on emacs-devel, if anywhere.

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

[Tim X]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Fri, 11 Mar 2011 10:52:14 -0500
>> 
>> So (save-excursion (goto-char BAR)) is pretty much a no-op.
>> 
>> But (save-excursion (set-buffer FOO) (goto-char BAR)) is either:
>> - the same as (save-excursion (goto-char BAR)), in case FOO is already current.
>> - an inefficient form of (with-current-buffer FOO (goto-char BAR)).
>> I still haven't found any code out there where this behavior is what
>> is actually wanted and expected by the programmer.
>> 
>> In more than 90% of the cases, the intended meaning is
>> (with-current-buffer FOO (goto-char BAR)) and the behavior if FOO is
>> current (to additionally preserve point) is harmless, so the warning
>> simply points out an inefficiency.
>> 
>> In the remaining cases, FOO is almost always current, but when it's not
>> the resulting behavior is a bug.  I.e. the intended code is
>> (with-current-buffer FOO (save-excursion (goto-char BAR))).
>
> Then how about changing the text of the warning to something like
>
>   Warning: `save-excursion' will not preserve point in the other buffer
>   set by `set-buffer'
>

I'm not sure it is possible to get a really concise warning message here
and the issue requires more explination than is possible in a warning
message.

My suggestion would be to add the word 'possibly' and perhaps add a
reference to a relevant section in the manual i.e.

Warning: save-excursion possibly defeated by set-buffer. See <relevant
elisp manual section>.

The above would be preferrable over longer warning messages, especially
messages that really only describe the behavior of save-excursion as
that doesn't really add anything the programmer probably didn't already
know. There are other warning which reference the manual (such as the
one concerning old style backquotes), so it seems like a consistent
approach. Adding the word possibly emphasises this is a warning and not
an error and could be legitimate.

Issuing a warning with a reference to a relevant section of the
manual would allow the programmer to see the warning, lookup that
section and get a more in-depth explination about the possible
situations and why either save-excursion is not doing what the
programmer expects or is inefficient and would be better coded using
with-current-buffer etc. They can then decide how relevant the warning
is and adapt their code appropriately. 

Tim

-- 
tcross (at) rapttech dot com dot au

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

[Drew Adams]

> > The mere fact that Eli Z and David K have very different 
> > interpretations of the potential problem(s)
> 
> I didn't interpret anything.  I just read what Stefan wrote, and
> perhaps misunderstood him (though I don't think so).

I stand corrected.  Eli has apparently not yet interpreted the warning message.
(Dunno if that helps.)

I should have said that Eli and David seem to have very different understandings
of Stefan's _explanation_ of what the warning message means.

That in itself does not argue very strongly in favor of such a warning, does it?

Stefan is the one behind the existence of this warning, AFAIK (and the wording?
dunno).  He has several times now tried to explain it to people who are confused
(or annoyed) by it.

And yet (after emacs-devel discussions on and off since 2009) Eli and David
apparently interpret Stefan's explanations very differently...

If the warning message is unclear (to some at least), and the explanations of
what the message is about are also unclear (to some seasoned Emacs vets at
least), then maybe that's a sign - no?

> Anyway, this entire discussion is misplaced.  The right place to
> discuss this warning is on emacs-devel, if anywhere.

1. We've been there and done that.

2. Emacs-devel is no longer the only appropriate place to discuss or explain
this warning.  Now that the warning is out there in the wild and they are coming
into contact with it, users too are asking about it (in help-gnu-emacs).

FWIW, personally I've made my peace with it.  I thought this controversy was
over and dead.  But users will continue to discover the warning anew.  I doubt
that we have heard the last of it, regardless of what the message might really
mean.

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

[Tim X]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> Am 12.03.2011 09:59, schrieb Eli Zaretskii:
>>> From: Stefan Monnier<monnier@iro.umontreal.ca>
>>> Date: Fri, 11 Mar 2011 10:52:14 -0500
>>>
>>> So (save-excursion (goto-char BAR)) is pretty much a no-op.
>>>
>>> But (save-excursion (set-buffer FOO) (goto-char BAR)) is either:
>>> - the same as (save-excursion (goto-char BAR)), in case FOO is already current.
>>> - an inefficient form of (with-current-buffer FOO (goto-char BAR)).
>>> I still haven't found any code out there where this behavior is what
>>> is actually wanted and expected by the programmer.
>>>
>>> In more than 90% of the cases, the intended meaning is
>>> (with-current-buffer FOO (goto-char BAR)) and the behavior if FOO is
>>> current (to additionally preserve point) is harmless, so the warning
>>> simply points out an inefficiency.
>>>
>>> In the remaining cases, FOO is almost always current, but when it's not
>>> the resulting behavior is a bug.  I.e. the intended code is
>>> (with-current-buffer FOO (save-excursion (goto-char BAR))).
>>
>> Then how about changing the text of the warning to something like
>>
>>    Warning: `save-excursion' will not preserve point in the other buffer
>>    set by `set-buffer'
>>
>> ?
>>
>>
>
>
>
> Seems all what's neccessary to know about save-excursion already is expressed
> in it's docstring.
>

Not sure thats true. I've seen a lot of elisp code that does the
save-excursion, set buffer pattern, which makes me think maybe the
documentation is inadequate. 

> If people don't want the buffer restored alongside with point and mark, they
> should not use this form.
>

I think it is a little more subtle than that. People may be using it
under the expectation that point and mark will also be saved in the
buffer accessed by set-buffer. 

> As I used it, compiler may assume I wanted that. No reason for a warning if
> used as provided.
>

Except we know it is often used incorrectly. I think Uday's point is
very valid here. When writing code, part of the aim is to communicate
intentions to others who may need to maintain or update the code in the
future. As Uday pointed out, using save-excursion followed by set-buffer
muddies the intention and decreases clarity. 

> OTOH we need compiler warnings for serious things.
>
> So my suggestion is simply to drop that warning.
>

I would vote to keep the warning, but modify to be 'possibly defeated'
and add a reference to the manual where the issue is clarified. 

If you have a situation where the pattern is legit, the warning can be
turned off. 

Tim

-- 
tcross (at) rapttech dot com dot au

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

[Uday Reddy]

On 3/12/2011 7:00 PM, Andreas Röhler wrote:

> OTOH we need compiler warnings for serious things.

Trust me, this is indeed a serious problem. Those of us that maintain 
large pieces of elisp code have months or years worth of work ahead to 
clean up those problems.

> So my suggestion is simply to drop that warning.

You can drop it yourself by removing 'suspicious from byte-compile-warnings.

Or, more simply, you can replace save-excursion in the problem cases by 
save-current-buffer and the warnings will go away.  You haven't said 
anything in response to this suggestion of mine.  Before asking Emacs 
developers to change the compiler, perhaps you should explain why you 
are not able to follow that suggestion?

Cheers,
Uday

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

[Uday Reddy]

On 3/12/2011 3:25 PM, David Kastrup wrote:

>> Frankly, I think the warning message is quite fine.  save-excursion is
>> trying to preserve the point (and mark), but only for the
>> current-buffer.  set-buffer is changing the current-buffer and, so,
>> the preservation of the point in the current-buffer is useless.
>
> There is little reason to warn about "useless" code.

Why is that?  I am quite happy to receive warnings about bad code.

>> If one wants a completely plain warning message, it could be:
>>
>>    "save-excursion has the effect of save-current-buffer"
>>
>> It doesn't say very much, but nobody will presumably argue about it.
>
> The most plausible theory I have for this thread is that you are
> collectively trying to pull my leg.
>
> If you give the user that warning, he'll say "great, just like I wanted
> it to do".

Ok, how about

    "save-current-buffer is a better choice than save-excursion"

> Why warn the user that "save-excursion" is doing what he wants it to do,
> when the actual problem is that it may do more, namely reverting an
> excursion happening unintendedly in the original buffer?

If an excursion is happening unintentionally that is definitely a bug! 
You probably mean that reverting it unintentionally is not so bad.  But 
I happen to think that it is.  save-excursion's should be as close as 
possible to the point movements they are protecting.  One shouldn't 
randomly wrap large pieces of bad code in save-excursion and hope that 
nobody will notice.  It is really hard to maintain such code.  The 
compiler warning is really welcome from my point of view.  I only wish 
that the compiler warning was there 20 years ago so that I wouldn't have 
to deal with that mess now.  But better late than never!

Cheers,
Uday

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

[Uday Reddy]

On 3/12/2011 7:56 PM, Drew Adams wrote:

> But this has already been expressed (by several people), and rejected, in
> various discussions in emacs-devel@gnu.org.

I believe I was the one that last raised it in emacs-devel and, after 
the explanations from Stefan Monnier and Stephen Turnbull, I was 
satisfied.  I told you quite explicitly that I was satisfied.

> IMO, this warning has produced _far_ more confusion than it has eliminated.  And
> that will no doubt continue to be the case going forward.

Yes, I agree that this saga will continue for quite some time.  But I 
wouldn't say that the warning has produced all this confusion.  The 
confusion about how to use save-excursion correctly has existed for a 
long time and continues to exist.  The warning is the messenger, not the 
cause of the confusion.

> FWIW, I also agree with Andreas that a "warning" is for something serious.  A
> warning is not the same thing as in informative message.  A warning _warns_ you
> about potential danger/damage.  Alarmism eventually results in the Chicken
> Little effect (aka Boy Cries "Wolf!").

This particular warning is in the 'suspicious category.  It signals that 
the code is suspicious.  I for one would be very wary of using anybody's 
code that generates such warnings.  The warnings would signal to me that 
the developers have not taken care to use the programming language 
correctly.  They have produced code that happens to work but could break 
easily when pulled and stretched.

Cheers,
Uday

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

[Andreas Röhler]

[  ... ]
> I think it is a little more subtle than that. People may be using it
> under the expectation that point and mark will also be saved in the
> buffer accessed by set-buffer.
>

Hi Tim, Hi Uday,

wherefrom you assume that?

AFAIU buffer, point and mark are stored the very moment 
`(save-excursion' cames in.

They should be restored if the parentese closes.
Every setting of buffer, mark and point between then doesn't matter as 
far as the buffer now current again is concerned.

Maybe there have been other `save-excursion' intercurse. But every one 
tells it's own story.

What's the difficulty?

Andreas

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

[Uday S Reddy]

Tim X writes:

> > Seems all what's neccessary to know about save-excursion already
> > is expressed in it's docstring.
> >
> 
> Not sure thats true. I've seen a lot of elisp code that does the
> save-excursion, set buffer pattern, which makes me think maybe the
> documentation is inadequate. 

It seems that reason for the widespread use of the
save-excursion/set-buffer pattern is actually quite simple.  Before
Emacs version 20.1, there was no other way to do it.
save-current-buffer was only introduced in 20.1.  So, there is a lot
of old code that uses the old pattern.  Now, there is also a lot of
new code that uses the old pattern because people learn their patterns
from the old code.

> > If people don't want the buffer restored alongside with point and mark, they
> > should not use this form.
> >
> 
> I think it is a little more subtle than that. People may be using it
> under the expectation that point and mark will also be saved in the
> buffer accessed by set-buffer. 

No, I don't actually think that.  (I notice that Andreas has
questioned this assumption too.)

If at all people are doing it consciously, they do it because they
think it is harmless.  You want to preserve the current-buffer.  But,
if you also preserve the point and mark, it seems like harmless
redundancy.  So, what is the big deal?

It takes quite a bit of insight into the code to understand that it is
actually *harmful* redundancy.  The unintentional uses of
save-excursion cover up potential bugs inside the code which don't get
noticed because of this redundant saving.

So, my test is pure and simple.  Convert all the save-excursion's that
the compiler complains about to save-current-buffer.  If your code
continues to run perfectly, you are my hero.  If your code falls over,
then you should accept that your code is buggy and it is only able to
stand up on the crutches of what you thought was "harmless
redundancy".  Then it is up to you whether you want to fix the code or
continue to live with it.  But there are many of us who won't touch
such code with a tadpole because we regard it as buggy.

> Except we know it is often used incorrectly. I think Uday's point is
> very valid here. When writing code, part of the aim is to communicate
> intentions to others who may need to maintain or update the code in the
> future. As Uday pointed out, using save-excursion followed by set-buffer
> muddies the intention and decreases clarity. 

Unfortunately, it is more than a question of clarity.  It is a
question of correctness.  Let me restate my example from Friday a bit
more explicitly

(defun my-beautiful-function ()
    (save-excursion
       (set-buffer B)
       (goto-char x)
       (setq a (find-something))
       ...))

(defun find-something ()
        (....
           (set-buffer A)
           (goto-char y)
         ...))

find-something is *buggy* here.  It is moving point in buffer A though
it wasn't supposed to.  However, my-beautiful-function works fine as
long as I call it in buffer A because the "harmlessly redundant"
save-excursion at its top-level restores A's point.

Tomorrow, you decide to call my-beautiful-function from some other
buffer C, and all hell breaks loose.  You suddenly find that the point
is moving in the unrelated buffer A and you have no idea why.

So, rewrite the code as follows:

(defun my-beautiful-function ()
    (save-current-buffer
       (set-buffer B)
       (goto-char x)
       ...
       (setq a (find-something))
       ...))

(defun find-something ()
        (....
           (save-current-buffer
              (set-buffer A)
              (save-excursion
                 (goto-char y)
                 ...)))

Not only would you have mollified the compiler, but you will have much
more robust code that will continue to function when used in
unforeseen ways.

Cheers,
Uday

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

[Uday Reddy]

On 3/13/2011 7:15 AM, Andreas Röhler wrote:

> They should be restored if the parentese closes.
> Every setting of buffer, mark and point between then doesn't matter as
> far as the buffer now current again is concerned.
>
> Maybe there have been other `save-excursion' intercurse. But every one
> tells it's own story.

I don't fully understand what you are trying to say.  But the advice 
given to me by Stephen Turnbull, which I fully agree with, is that 
save-excursion should be placed *as close as possible* to the point 
movements that it is trying to revert.  Placing it far away may result 
in the connection being broken sometime down the road when the code is 
modified or reused in new ways.

If there is a function that has the side-effect of changing point or 
mark, then ask yourself whether that is the intended effect of the 
function.  If you depend on an unrelated save-excursion to restore the 
point movement, then you will be asking for trouble.

Another way to think about the problem is, for every save-excursion in 
the code:

- are you clear which point movements it is trying to revert,

- is it placed as close as possible to those point movements?

Cheers,
Uday

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

[Uday Reddy]

On 3/3/2011 4:58 AM, Le Wang wrote:

> Either the compiler warning or the manual is wrong
> (http://www.gnu.org/software/emacs/elisp/html_node/Current-Buffer.html):
>
>     Therefore, you should normally use |set-buffer| within a
>     |save-current-buffer| or |_save-excursion_| (see Excursions
>     <http://www.gnu.org/software/emacs/elisp/html_node/Excursions.html#Excursions>)
>     form that will restore the current buffer when your function is
>     done. Here, as an example, is a simplified version of the command
>     |append-to-buffer|:

That is a fair point.  It seems that this section of the manual hasn't 
been edited for a long time.  I am submitting some changes now.

Notice, however, that the section on save-excursion at
 
http://www.gnu.org/software/emacs/elisp/html_node/Excursions.html#Excursions
says:

       When only the identity of the current buffer needs to be saved
       and restored, it is preferable to use save-current-buffer instead.

Cheers,
Uday

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

[Drew Adams]

> > But this has already been expressed (by several people), 
> > and rejected, in various discussions in emacs-devel@gnu.org.
> 
> I believe I was the one that last raised it in emacs-devel and, after 
> the explanations from Stefan Monnier and Stephen Turnbull, I was 
> satisfied.  I told you quite explicitly that I was satisfied.

So?  As I said, the warning was discussed, and getting rid of it was rejected.
You are convinced that that's a great decision; I am not.  No way of knowing
what most people felt or feel, and it doesn't matter because polls are not taken
anymore when deciding...  (Poor Emacs.)

> > IMO, this warning has produced _far_ more confusion than it 
> > has eliminated.  And that will no doubt continue to be the
> > case going forward.
> 
> Yes, I agree that this saga will continue for quite some time.  But I 
> wouldn't say that the warning has produced all this confusion.  The 
> confusion about how to use save-excursion correctly has existed for a 
> long time and continues to exist.  The warning is the 
> messenger, not the cause of the confusion.

I'm not convinced of the existence of this bogeyman about mass confusion
concerning how to use `save-excursion'.  I don't see such confusion.

But it's pretty _obvious_ that the warning message has produced a colossal
amount of confusion.  Everybody and her brother has a different understanding of
what the "danger" might be.

> > FWIW, I also agree with Andreas that a "warning" is for 
> > something serious.  A warning is not the same thing as in
> > informative message.  A warning _warns_ you about potential
> > danger/damage.  Alarmism eventually results in the Chicken
> > Little effect (aka Boy Cries "Wolf!").
> 
> This particular warning is in the 'suspicious category.
> It signals that the code is suspicious.

Only if you think that using `save-excursion' with `set-buffer' is suspicious,
which in general it is not.

Wrt the supposed danger, David K replied this to Stefan (who replied to me,
defining the danger)
http://lists.gnu.org/archive/html/emacs-devel/2010-01/msg00588.html:

>>> 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.

David is correct of course.  And Stefan's explanation of the "danger" does not
indicate any danger, in any case.  Where's the beef?

> I for one would be very wary of using anybody's code that generates
> such warnings.

That is precisely one of the problems created by this misguided message.  Users
get freaked out seeing WARNINGs when they byte-compile the code.  They don't
understand the message (even seasoned developers differ in their guesses as to
its meaning), and the unknown scares them.

Even Stefan admits that the compiler is incapable of issuing such warnings only
when the code is in fact problematic in his eyes.  In plain English, it's stupid
and warns about situations that even Stefan admits are not a problem.  And much
of the `save-excursion' code that he would consider problematic is in fact
typically safe, sound, and sure.

IOW, the warnings are all over the map - "tales told by an idiot, full of sound
and fury, signifying nothing".

Whether or not every use of `save-excursion' with `set-buffer' is stylistically
neat or is the best choice in terms of software engineering is a different
matter.  And it is a matter that is not handled discriminately by this message,
in any case.

> The warnings would signal to me that the developers have not
> taken care to use the programming language correctly.

If they automatically signal that to you, then I would suggest that you might
not have taken sufficient care to learn the programming language correctly. ;-)

More seriously, RMS et al who created `save-excursion' were not idiots.  They
explicitly designed it to save and restore which buffer is current, in addition
to its point and mark.  Why, do you suppose?

And most users of it over the years have used it well - without any warnings.
If you automatically think that such a warning indicates bad code or inadequate
knowledge of Emacs Lisp then you are jumping to a wild, indiscriminate
conclusion, IMO.

> They have produced code that happens to work but 
> could break easily when pulled and stretched.

You cannot deduce that from the warnings issued.  They might have; they might
not have.  Not every use of `save-excursion' with `set-buffer' indicates
fragile, messy, or otherwise unwise code.

Far from it.  You yourself mentioned zillions of such uses in the code you
inherited, only a tiny minority of which you felt needed correcting.  And you
certainly didn't need a warning for each occurrence in order to search your code
for possible improvements.

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

[Drew Adams]

> > I think it is a little more subtle than that. People may be using it
> > under the expectation that point and mark will also be saved in the
> > buffer accessed by set-buffer.
> 
> wherefrom you assume that?  AFAIU buffer, point and mark are
> stored the very moment `(save-excursion' cames in.
> 
> They should be restored if the parentese closes.
> Every setting of buffer, mark and point between then doesn't 
> matter as far as the buffer now current again is concerned.
> 
> Maybe there have been other `save-excursion' intercurse. But 
> every one tells it's own story.

Exactly.  Andreas is exactly right.  The emperor has no clothes.

> What's the difficulty?

You're right, Andreas - there isn't any.  The doc string of `save-excursion'
says it all, and quite clearly too.

The only possible confusion it lets in, IMO, is via the wording "save...current
buffer" in the first line.  But it then more carefully makes clear that the
buffer _content_ is not _saved_ as in saving to a file; instead, what is "saved"
(remembered) is which buffer is current, and that buffer is made current again
at the end.  And that possible confusion exists just as much - no, more, for the
innocent, praised, and now unduly privileged `save-current-buffer'.

But the issue of that possible confusion has not even been raised by those who
want to warn us about using `save-excursion' with `set-buffer'.  The purported
"problems" they cite have nothing to do with the ambiguous wording about
"saving" the current buffer.

They have to do with users supposedly assuming that `set-buffer' itself saves or
restores state (which buffer is current, or point, or mark) - put differently,
that a `set-buffer' somehow overrides a surrounding `save-excursion', preventing
it from restoring things.  These "problems" have nothing to do with
`save-excursion' or its doc.  If there is any teaching to be done about such
"problems" then it should be done in the doc of `set-buffer'.

On the other hand, Stefan has also claimed that users think that
`save-excursion' will also restore point in other buffers, in particular, in the
`set-buffer' buffers.  Again, the `save-excursion' doc is very clear about this.
If users are confused in this way (no evidence was given) then teach them with
doc, not with blanket warnings that say nothing.

Supposedly people are making these false assumptions all over the place and the
result is buggy code.  Users purportedly think that `save-excursion' does not
restore the point or the mark if there is a `set-buffer' in the body, that the
`set-buffer' takes precedence and any movements after it leave the point or mark
changed beyond the end of the excursion.  Or they purportedly think that
`save-excursion' restores point and mark in more than one buffer.  IOW, users
can't be trusted to read well and code well; they need to be warned.

The variants of the first purported misconception (`set-buffer' overrides
`save-excursion') have to do with whether the target of `set-buffer' is the same
buffer as was saved by `save-excursion'.  Supposedly `save-current-buffer' is
"safer" because it (correctly) leads no one to suppose that point and mark will
be restored.

The mildest variant of this problem is when the user doesn't really want or need
to save and restore point and mark - s?he just wants to restore which buffer is
current.  Stefan says that using `save-excursion' instead of
`save-current-buffer' or `with-current-buffer' is then "inefficient".  Well,
sure, a little bit...  It's (inexpensive) overkill.  But a _warning_?

I have not seen that people are willy-nilly falsely assuming that a `set-buffer'
inside a `save-excursion' overrides it.

I'm sure that a few such bugs have been introduced somewhere, but this kind of
thing can happen also with (let ((a b))...(setq a c)...).  We do not _warn_
people that "`let' defeats `setq'" just because `let' restores a value set by
`setq'.  (Yes, I know the analogy is imperfect; I know there are different `a's
etc.)  We expect users to read the `let' doc and learn to use it correctly.

I also have not seen that people falsely assume that `save-excursion' restores
point for more than the buffer that was current when it was invoked.  Maybe
someone has misunderstood this way, but issue a _warning_ each time
`save-excursion' is used with `set-buffer'?

`save-excursion' simply remembers where you are and returns there when the
excursion is finished.  End of story.  The "where you are" includes the buffer,
your position in that buffer (point), the mark (if any) in that buffer, and
whether the mark was active.

If a non-local exit (e.g. `top-level' or `throw' outside the `save-excursion')
occurs while evaluating the `save-excursion' body then the rest of that body is
not evaluated (naturally).  But the saved information is still restored (which
buffer is current, plus its value of point and mark).  That completes the
behavior of `save-excursion'.  And all of the behavior is explained in the doc
string.

If someone is convinced that `save-excursion' is evil or is too hard for
Emacs-Lisp users to figure out, and s?he wants to teach better programming
practices, the way to do that is not to set off the fire alarm each time code
uses `set-buffer' inside `set-excursion'.

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

[Drew Adams]

> > They should be restored if the parentese closes.
> > Every setting of buffer, mark and point between then 
> > doesn't matter as far as the buffer now current again
> > is concerned.
> >
> > Maybe there have been other `save-excursion' intercurse. 
> > But every one tells it's own story.
> 
> I don't fully understand what you are trying to say.

He's saying that each `save-excursion' does the same thing: it remembers which
buffer is current, and point and mark in that buffer, and then it restores them.
Nothing more.

That's very simple.  It doesn't matter how many `save-excursion's there are or
how convoluted the code might be: when it finishes, each restores what it
remembered.

> save-excursion should be placed *as close as possible* to the point 
> movements that it is trying to revert.

Agreed.  I don't think anyone disputes that.

But you seem to think that `save-excursion' should never have been defined to
remember which buffer is current and restore that.  `save-excursion' is _not_
only about point movement.  It is not designed only to save point and mark in
the current buffer.  It is _designed_ to also restore that buffer, making it
current again.

> Placing it far away may result in the connection being broken
> sometime down the road when the code is modified or reused in new ways.

Again, that's common sense programming practice.  Good to remember, sometimes
forgotten at one's peril.  We all agree AFAIK.

> Another way to think about the problem is, for every 
> save-excursion in the code:
> - are you clear which point movements it is trying to revert,
> - is it placed as close as possible to those point movements?

Again, no disagreement.

Though `save-excursion' is not only about point movement.  And even wrt point
movement it really doesn't matter which movements a `save-excursion' is "trying
to revert".  It is trying to revert them all (in its body) - and it does.

Your real point here, I think, is again that it helps understanding and
modularity to keep the scope of a `save-excursion' as tight as possible.
Everyone agrees AFAIK.

The same is true of other Lisp constructs, starting with `let'.  There is
nothing in any of this that argues for a _WARNING_ about `save-excursion'
"defeating" `set-buffer'.

And you seem to have missed that `save-excursion' is about buffer restoration,
not just point restoration.  Or perhaps you want to redefine it to not restore
the buffer.

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

[Drew Adams]

> > OTOH we need compiler warnings for serious things.
> 
> Trust me, 

I don't distrust you, Uday.

> this is indeed a serious problem.

Nonsense.  You seem to have drunk the kool-aid. ;-)
And no, it's not about trusting you.

Warnings are about possible danger or damage.
Pseudo warnings such as this one just dilute the meaning.

And since this one is also incomprehensible, there is no meaning conveyed except
abstract fear of some unknown danger.  This is a bogeyman message.

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

[Uday Reddy]

On 3/13/2011 6:25 PM, Drew Adams wrote:

>
>> save-excursion should be placed *as close as possible* to the point
>> movements that it is trying to revert.
>
> Agreed.  I don't think anyone disputes that.

Oh, good.  We are getting somewhere.

Now, let us take the next step.  Every save-excursion/set-buffer 
combination is a remote save-excursion.  It is trying to protect point 
movements that are far away in code, so far away in fact that it is 
extremely hard to find them.

So, by objecting to the compiler warning, you are indeed disputing the 
principle.

> But you seem to think that `save-excursion' should never have been defined to
> remember which buffer is current and restore that.  `save-excursion' is _not_
> only about point movement.  It is not designed only to save point and mark in
> the current buffer.  It is _designed_ to also restore that buffer, making it
> current again.

In retrospect, it seems that that design was a mistake.  It would have 
been far better to have separate primitives for save-current-buffer and 
save-point-and-mark.  Now that we have save-current-buffer available, 
there is no reason to use save-excursion to restore the current buffer. 
  If you know of such reasons, please tell me.

>> Another way to think about the problem is, for every
>> save-excursion in the code:
>> - are you clear which point movements it is trying to revert,
>> - is it placed as close as possible to those point movements?
>
> Again, no disagreement.
>
> Though `save-excursion' is not only about point movement.  And even wrt point
> movement it really doesn't matter which movements a `save-excursion' is "trying
> to revert".  It is trying to revert them all (in its body) - and it does.
>
> Your real point here, I think, is again that it helps understanding and
> modularity to keep the scope of a `save-excursion' as tight as possible.
> Everyone agrees AFAIK.

If you are agreeing, then it seems that your disagreement is about 
whether the compiler should help people in getting their 
save-excursion's right.  You are not disagreeing that the compiler is 
being helpful.  You seem to be saying that the compiler has no business 
in doing such things.  Is that what this dispute really about?

And, no, I haven't "missed" that save-excursion restores the current 
buffer.  But save-current-buffer restores it just as well, and so that 
aspect of save-excursion doesn't seem relevant.  If you want to argue 
that there are cases where save-excursion is a better choice than 
save-current-buffer, then by all means let us discuss that!

Cheers,
Uday

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

[Drew Adams]

> >> save-excursion should be placed *as close as possible* to the point
> >> movements that it is trying to revert.
> >
> > Agreed.  I don't think anyone disputes that.
> 
> Oh, good.  We are getting somewhere.

No, you are just stating the obvious.

> Now, let us take the next step.  Every save-excursion/set-buffer 
> combination is a remote save-excursion.  It is trying to 
> protect point movements that are far away in code, so far away
> in fact that it is extremely hard to find them.

Not at all true.  Doesn't follow at all.

And you don't seem to realize yet that `save-excursion' has no effect on point
in any buffer other than the original one that it makes current again when it's
done.

> So, by objecting to the compiler warning, you are indeed 
> disputing the principle.

Your "principle" has nothing to do with the compiler warning or `save-excursion'
or `set-buffer'.  It is simply the idea that there is no sense including things
in scope that you do not need to include in scope.

I might advise someone not to put one giant `let' at the beginning of his
function, suggesting that he instead use multiple `let's, each with limited
scope and a limited set of variables.  IOW, shrink-fit the scopes to what really
needs scoping in each case.

But that does not call for a warning or imply any danger.  Yes, even if doing
things such a messy way would in the long run likely lead to maintenance
problems and introduce errors.  No need for _alarm_, still.  And no need to
confuse people who are trying to understand `save-excursion' by mixing in such a
mention of scope.

Whether to use `save-excursion' with `set-buffer' has to do with whether you
want to restore which buffer was current and its point and mark, after doing
something in some buffer - possibly point movements and possibly the same
buffer.  Nothing more.

Compiler advice not to use some particular occurrence of `save-excursion' with
`set-buffer' should point to a real or a likely problem.

If all the compiler really means to say each time you use these two functions
together is "Have you made the `save-excursion' scope as tight as possible?",
then it's wasting its time and the user's.  In that case it might as well be
reminding us "Have a nice day" or "Don't forget to brush your teeth".

> > But you seem to think that `save-excursion' should never 
> > have been defined to remember which buffer is current and
> > restore that.   `save-excursion' is _not_ only about point
> > movement.  It is not designed only to save point and mark in
> > the current buffer.  It is _designed_ to also restore that 
> > buffer, making it current again.
> 
> In retrospect, it seems that that design was a mistake.

Whose retrospect?  Do you hear RMS saying he made a mistake?

You and I each have our own right to judge, of course, regardless of what RMS
might think now, in retrospect.  But you and I disagree.

> It would have been far better

Far better?

> to have separate primitives for save-current-buffer and 
> save-point-and-mark.

That was already proposed by Stephen T.  And Stefan disagreed that it was even a
good idea, let alone a "far better" idea.

Myself, I don't have a position on it - haven't really thought about it.  But
YAGNI as long as `save-excursion' is available.

> Now that we have save-current-buffer available, 
> there is no reason to use save-excursion to restore the 
> current buffer.  If you know of such reasons, please tell me.

There is no reason to use it to do only what `save-current-buffer' does.  There
_is_ a reason to use it to do what _it_ does, which is to also restore the
current buffer's point and mark.

> > Your real point here, I think, is again that it helps 
> > understanding and modularity to keep the scope of a
> > `save-excursion' as tight as possible.  Everyone agrees AFAIK.
> 
> If you are agreeing, 

I am, but you are not!  You just made it pretty clear that you are against using
`save-excursion' with `set-buffer' at all!

> then it seems that your disagreement is about 
> whether the compiler should help people in getting their 
> save-excursion's right.

The compiler does not at all help people get their `save-excursion's right.  And
neither does your proposed change to the manual (on emacs-devel@gnu.org).

The way to help people get their `save-excursion's right, i.e., to avoid the
"problems" that Stefan has referred to, is to document `save-excursion' well and
fix any relevant mistakes in the Emacs source code.

And `save-excursion' is already documented well.  But that doesn't guarantee
that some people won't neglect to read the doc or won't misread it.

You've hinted a few times now that you might not realize that `save-excursion'
does not restore point or mark for any buffer other than the original one.  And
a couple of times now you seem to have missed or forgotten that it also restores
which buffer was current (yes, I see that you disagree below).

And if even _you_ could miss such things from reading what is a clear
explanation then no, there are no guarantees against THE DANGER.

Still, as you admitted, in practice most uses have been sane.  We can thus
generally give users the benefit of the doubt that they can understand the doc
string and figure out when and how to use the function.

> You are not disagreeing that the compiler is being helpful. 

Oh yes I am!  It is not only not being helpful by issuing such warnings; it is
being downright harmful.  I've given several reasons/examples already.

> You seem to be saying that the compiler has no business 
> in doing such things. 

What are "such things"?  Unless it can do something well, helping more than
hurting, then it has no business doing _anything_.  Advising about scope extent
seems to fall into that category, at least now.  Anyway, I don't see you
fighting to have the compiler analyze whether your code could benefit from
tighter `let' scoping.

And the compiler has no business giving the impression that using
`save-excursion' with `set-buffer' is a no-no.

And it certainly has no business scaring people without conveying any info about
the purported DANGER.

> And, no, I haven't "missed" that save-excursion restores the current 
> buffer.  But save-current-buffer restores it just as well, 
> and so that aspect of save-excursion doesn't seem relevant.

It's relevant to any case where you want to restore which buffer is current AND
its point and mark.  That's the use case that you seem to be missing/ignoring.
It seems to be the elephant in the room.

> If you want to argue that there are cases where save-excursion is
> a better choice than save-current-buffer, then by all means let us
> discuss that!

`save-current-buffer' does not restore point and mark.  If you want to restore
them as well as which buffer is current, then use `save-excursion'.  That's what
it's for. QED.

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

[Uday Reddy]

On 3/13/2011 6:22 PM, Drew Adams wrote:

> So?  As I said, the warning was discussed, and getting rid of it was rejected.
> You are convinced that that's a great decision; I am not.  No way of knowing
> what most people felt or feel, and it doesn't matter because polls are not taken
> anymore when deciding...  (Poor Emacs.)

No, polls are not in.  It was pointed out that the Gnu project is not a 
democracy.  It is an enlightened dictatorship in the Socratic style.  I 
am a great fan of Socrates myself and I wouldn't want the Gnu project to 
go down the tube like the Athenian democracy did.

>> This particular warning is in the 'suspicious category.
>> It signals that the code is suspicious.
>
> Only if you think that using `save-excursion' with `set-buffer' is suspicious,
> which in general it is not.

David K's example in the message you cite below is one that involves 
user interaction.  So, you can't wrap save-excursion around it.  One can 
hardly generalize from such a special case.

> Wrt the supposed danger, David K replied this to Stefan (who replied to me,
> defining the danger)
> http://lists.gnu.org/archive/html/emacs-devel/2010-01/msg00588.html:
 >
>>>> 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.

I don't see anything "wrong" in Stefan's point.  Whether the code has a 
side effect or not depends on which buffer it is running in.  It has a 
side effect on any buffer except the current buffer.  That is strange 
and buggy code.  Stefan's point stands.

I am not sure what David's point is. actually.  "It will always restore 
the original buffer and its point."  But, what is so special about the 
original buffer?  Is it fine for the code to muck up every buffer other 
than the original buffer?

> David is correct of course.  And Stefan's explanation of the "danger" does not
> indicate any danger, in any case.  Where's the beef?

I haven't seen Stefan use words like "danger".  He said that it led to 
buggy code.  Here is a direct quote:

"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."

>> I for one would be very wary of using anybody's code that generates
>> such warnings.
>
> That is precisely one of the problems created by this misguided message.  Users
> get freaked out seeing WARNINGs when they byte-compile the code.  They don't
> understand the message (even seasoned developers differ in their guesses as to
> its meaning), and the unknown scares them.

I agree that the users/developers need to be given more help.  That is 
why I have been working on edits to the manual.

I probably overstated my wariness in my previous message.  Code that 
generates the "suspicious" warnings is not necessarily dysfunctional. 
It is just fragile.  It can break when modified or extended.  But what I 
would be really wary of developers that fail to understand the issue and 
blame it all on the compiler stupidity instead.

> More seriously, RMS et al who created `save-excursion' were not idiots.  They
> explicitly designed it to save and restore which buffer is current, in addition
> to its point and mark.  Why, do you suppose?

They designed them in the early days and those were good first-cut 
solutions.  But we learn from experience and make progress.  Now, 
`save-current-buffer' represents one half of `save-excursion', and it is 
exactly the half that is relevant to `set-buffer'.  So, when the elisp 
gurus recommend that you use that instead of `save-excursion', why is 
there this resistance?  Why these endless debates?

> And most users of it over the years have used it well - without any warnings.
> If you automatically think that such a warning indicates bad code or inadequate
> knowledge of Emacs Lisp then you are jumping to a wild, indiscriminate
> conclusion, IMO.

They haven't used it well.  It is really not possible to use it 
correctly.  Slapping a save-excursion at the top level hides all the 
side effects and you don't know what unwanted side effects have occurred 
inside the code.  As you know, I maintain VM which was developed by a 
star programmer like Kyle Jones.  But if I replace its save-excursion's 
by save-current-buffer's, it falls down pretty badly, with the buffer 
points moving in all kinds of unpredictable ways.  For all I know, there 
might be only one or two unprotected point movements in there.  But they 
are enough to wreak havoc.  Tracking them down is incredibly hard.  The 
best I can do is to replace a few save-excursions at a time and watch 
carefully to see if anything goes wrong.  It will take me a long time to 
get rid of all the save-excursions.

So, my best advice would be, don't do it.  Don't use save-excursion with 
set-buffer.  It is completely illogical.  There is no need for it.  The 
compiler is right.  Please pay attention.

Cheers,
Uday

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

[Drew Adams]

> > No way of knowing what most people felt or feel, and it
> > doesn't matter because polls are not taken anymore when
> > deciding...  (Poor Emacs.)
> 
> No, polls are not in.  It was pointed out that the Gnu 
> project is not a democracy.  It is an enlightened dictatorship
> in the Socratic style.  I am a great fan of Socrates myself and
> I wouldn't want the Gnu project to go down the tube like the
> Athenian democracy did.

Tell it to Enlightened Dictator RMS.  He's the one who's most often calling for
Emacs Dev to poll the users these days, as had been done sometimes during His
reign.  Maybe you should explain to him that listening to him about polling
would send the GNU project down the tube.

> > Wrt the supposed danger, David K replied this to Stefan 
> > (who replied to me, defining the danger)
> > http://lists.gnu.org/archive/html/emacs-devel/2010-01/msg00588.html:
> >
> >>>> 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.
> 
> I don't see anything "wrong" in Stefan's point.  Whether the 
> code has a side effect or not depends on which buffer it is
> running in. It has a side effect on any buffer except the
> current buffer.  That is strange and buggy code.  Stefan's
> point stands.

What side effect are you talking about?  Picking up info and assigning it to a
var?  That happens regardless of which buffer was originally current.  Restoring
point?  That happens on _no_ buffer except the originally current one.  And it
happens there regardless of which buffer that was (even if it was FOO or BAR).

There is nothing strange or buggy about that code.  `save-excursion' here, as
always, does just what David said: it makes the original buffer current again
and restores its point and mark.

More to the point, there is certainly nothing DANGEROUS here, which is what
Stefan claimed.  This was supposedly his _definition_ of the DANGER involved
with using `save-excursion' with `set-buffer' - the reason for issuing a
WARNING.  "What is dangerous about it is..." - but no danger was indicated.

> I am not sure what David's point is. actually.  "It will 
> always restore the original buffer and its point."

That's the point.  The behavior of `save-excursion' does not depend on which
buffer was originally current.  It does not do something different if FOO was
current or TOTO was.  It _always_ restores the original buffer and its point,
whatever that buffer might have been.

Stefan claims that it is "dangerous" that "the point-saving" is a no-op for any
buffer other than the originally current one.  And that's the exact job
`save-excursion' has; it's exactly what it is designed to do.  Claiming that is
tantamount to claiming that `save-excursion' is dangerous whenever you make
another buffer current somewhere in its body.

> But, what is so special about the original buffer?  Is it fine
> for the code to muck up every buffer other than the original buffer?

What on earth are you talking about?  What is being mucked up?

The _point_ of `save-excursion' is to restore the original buffer and its point
(and mark) - nothing more.  It has never had as object to do the same for any
other buffers.  The doc is very clear about what it does.  Stefan might think
that some people get fooled into thinking that it saves and restores other
buffers and their points, but he's shown no evidence for that.  And no evidence
of any "danger".

> > David is correct of course.  And Stefan's explanation of the "danger"
> > does not indicate any danger, in any case.  Where's the beef?
> 
> I haven't seen Stefan use words like "danger".

You haven't?  Look above.  "What is dangerous about it is..."  He is answering
the direct question of what's so dangerous.

> I probably overstated my wariness in my previous message.  Code that 
> generates the "suspicious" warnings is not necessarily dysfunctional. 
> It is just fragile.  It can break when modified or extended.  
> But what I would be really wary of developers that fail to understand 
> the issue and blame it all on the compiler stupidity instead.

Failing to understand `save-excursion' is what this is all about, AFAICT.  There
is no reason to expect it to save and restore anything to do with any buffer
other than the original one.  The doc is very clear about what it does.  It is
not a bug that `save-excurstion' does not prevent you from "mucking up" anything
you might want to.  That's not its job.

> > More seriously, RMS et al who created `save-excursion' were 
> > not idiots.  They explicitly designed it to save and restore
> > which buffer is current, in addition to its point and mark.
> > Why, do you suppose?
> 
> They designed them in the early days and those were good first-cut 
> solutions.

A first-cut that is several decades old now.  Tried and true, I'd say.  Why do
you suppose they had it save and restore point and mark?  What were they
thinking?

There is always room for improvement to Emacs, of course.  But no problem with
`save-excursion' has yet been pointed out - beyond a claim that some users might
somehow expect it to save and restore additional buffers.

> But we learn from experience and make progress.  Now, 
> `save-current-buffer' represents one half of `save-excursion',
> and it is exactly the half that is relevant to `set-buffer'.  So,
> when the elisp gurus recommend that you use that instead of
> `save-excursion', why is there this resistance?  Why these endless
> debates?

First, no one has disagreed that `save-current-buffer' and `with-current-buffer'
are appropriate when you only want to save which buffer is current, and not its
point.

Second, "the elisp gurus" are not lined up as a group behind this warning, even
if it has been accepted as a fait accompli.  And you saw in this very thread
recently that two such gurus (Eli, David K) have _very_ different understandings
of what the warning is even trying to convey.

Third, the debate about the warning ended in emacs-devel months ago (a year
ago).  Users are now coming in contact with the incomprehensible warning for the
first time and have therefore raised the question (WTF?) in help-gnu-emacs.

_You_ then brought it back to emacs-devel by proposing a change to the manual
that distorts things (IMHO), removing `save-excursion' from the passages that
discussed its use.  Yes, I know that you were honestly trying to help explain
the message, but you can see how that has caused a new discussion of what the
warning means (what "problems" or "dangers" it tries to avoid).

It is unlikely that the confusion introduced by this warning message will go
away soon, as users discover it anew.  Get the gurus to agree about what the
message really means - what the problem to be avoided is, and then maybe you can
add the result to the manual to clear up the message in a way that won't stir
the pot.  And perhaps at the same time improve the message based on agreement of
what it means.

> > And most users of it over the years have used it well - 
> > without any warnings.  If you automatically think that such a
> > warning indicates bad code or inadequate knowledge of Emacs Lisp
> > then you are jumping to a wild, indiscriminate conclusion, IMO.
> 
> They haven't used it well.

And the proof is?

> It is really not possible to use it correctly.

Well, here you and Stefan apparently disagree.  And you seem to disagree with
yourself as well, as you indicated that only a small minority of the occurrences
of `save-excursion' plus `set-buffer' in your VM code needed to be fixed.
(Stefan said the same of the Emacs sources; they have already been fixed, IIUC.)

> Slapping a save-excursion at the top level hides all the 
> side effects and you don't know what unwanted side effects 
> have occurred inside the code.

No idea what you're referring to.

> As you know, I maintain VM which was developed by a star programmer
> like Kyle Jones.  But if I replace its save-excursion's 
> by save-current-buffer's, it falls down pretty badly, with the buffer 
> points moving in all kinds of unpredictable ways.

Not surprising.  `save-current-buffer' is not `save-excursion'.

> For all I know, there might be only one or two unprotected
> point movements in there.  But they are enough to wreak havoc.

Were they wreaking havoc before you replaced `save-excursion' willy-nilly with
`save-current-buffer'?

> Tracking them down is incredibly hard.  The best I can do is to
> replace a few save-excursions at a time and watch carefully to see
> if anything goes wrong.  It will take me a long time to get rid of
> all the save-excursions.

And you are trying to get rid of all the `save-excursion's why?  Did someone
scare you into thinking that `save-excursion' is evil?  Stefan disagrees, FWIW.
 
> So, my best advice would be, don't do it.  Don't use save-excursion
> with set-buffer.

My advice would be _not_ to try willy-nilly getting rid of `save-excursion'.
It's probably there in your code for a reason.  If you are sure you understand
each use and you know that a given use can be changed to `save-current-buffer'
or `with-current-buffer' with no loss, then go for it.  But it doesn't sound
like you are too sure of that.

> It is completely illogical.  There is no need for it.  The 
> compiler is right.  Please pay attention.

Sheesh.  Give us a break.  Please pay attention yourself.

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

[Drew Adams]

> It takes quite a bit of insight into the code to understand that it is
> actually *harmful* redundancy.

What redundancy?  What harm?

> The unintentional uses of save-excursion cover up potential
> bugs inside the code which don't get noticed because of this
> redundant saving.

Redundant saving?  What redundant saving?  Each use of `save-excursion' saves
and restores the buffer that was originally current along with its point and
mark.  Where's the redundancy?  Where's the harm?

> So, my test is pure and simple.  Convert all the save-excursion's that
> the compiler complains about to save-current-buffer.

OMG.

> If your code continues to run perfectly, you are my hero.
> If your code falls over, then you should accept that your code is
> buggy

If it wasn't before, it probably is after your near-global search-and-replace.

And what if your code doesn't seem to fall over immediately, and it takes you 2
years to discover that it was misguided after all not to save and restore point
in that particular case?

> and it is only able to stand up on the crutches of what you thought
> was "harmless redundancy".

Please tell us more about the harmless redundancy and the harmful redundancy.
Or even just about the redundancy.

> Then it is up to you whether you want to fix the code or
> continue to live with it.  But there are many of us who won't touch
> such code with a tadpole because we regard it as buggy.
>
> Unfortunately, it is more than a question of clarity.  It is a
> question of correctness.  Let me restate my example from Friday a bit
> more explicitly
> 
> (defun my-beautiful-function ()
>     (save-excursion
>        (set-buffer B)
>        (goto-char x)
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
>            (set-buffer A)
>            (goto-char y)
>          ...))
> 
> find-something is *buggy* here.  It is moving point in buffer A though
> it wasn't supposed to.

Why wasn't it supposed to?  How can you know from this skeleton whether it was
supposed to move point in A or not?

Or are you just telling us that it is _hypothetically_ buggy, that
_hypothetically_ it is not supposed to move point in A, but it does?

Tell us more.  What buffer is `m-b-f' called in?  Presumably, if it is intended
to be called in buffer A, then it wants to restore point in A when it is done
(assuming the `save-excursion' is the last thing it does).

Presumably, if it is intended instead to be called in buffer C then it wants to
restore point in C when it is done.  In this case, presumably (if we don't just
assume that it is buggy) `find-something' wants to leave point in A at Y
(assuming it doesn't move it in the last `...').

There is no way to know from your skeleton what the purpose or the behavior is.
The devil is in the details...which are missing.

There is nothing intrinsically wrong with a function that moves point to some
position in some buffer and leaves it there.  I probably wouldn't name such a
function just `find-something', however, and I would document that part of its
purpose is to move point in buffer A or whatever.  Assuming that is the case.

> However, my-beautiful-function works fine as
> long as I call it in buffer A because the "harmlessly redundant"
> save-excursion at its top-level restores A's point.

Oh, so you were presuming that `m-b-f' wants to restore A's point.  OK.  Where's
the problem?  (So far nothing is _redundant_, however.)

If `m-b-f' intends to always do things in buffer A and afterward restore point
in A to what it was initially, and if it doesn't know which buffer it will be
called from, then it should wrap (with-current-buffer A ...) around its
`save-excursion'.

> Tomorrow, you decide to call my-beautiful-function from some other
> buffer C, and all hell breaks loose.  You suddenly find that the point
> is moving in the unrelated buffer A and you have no idea why.

Well you should know.  You should know from the name of `find-something' (which
needs a better name) and from its doc that it moves point in A.

Or if by hypothesis `f-s' is buggy and should not in fact move point in A, then
you should be able to see by debugging that it in fact does move point in A,
even when `m-b-f' is called in A.  And you should be able to tell just by
looking at the code that it moves point in A.

I don't deny that if the overall code is complex such a bug might not be
obvious, but I submit that this is a general problem that has nothing special to
do with `save-excursion' or `set-buffer'.  It is simply about keeping track of
the side effects performed by your code.

`find-something' as written has the side effect of moving point in buffer A -
always.  If that is not the intention, then yes, its bug will not surface as
long as it is called inside a `save-excursion' for buffer A.

The same problem would exist for a function that increments a variable X by 10
and is called by a function within a `let' that binds it to 42.  You might never
notice the incrementing as long as that `let' binding is there.  Call the
incrementing function from another context and you might suddenly discover the
incrementing.

There are a zillion cases of such things in a language like Lisp.  Given both
its side effects and dynamic binding there are all kinds of possible pitfalls in
Lisp.

It should be very clear that a construct that restores point for the current
buffer will restore point for the current buffer (!).  Restoring point for a
given buffer hides point movements that took place between saving and restoring
- of course.  The same would be true if you instead used your proposed
`save-point-and-mark' to do the saving and restoring.  Nothing remarkable here
at all.

Nothing mysterious.  Except for the various `...' and the context of use
(invocation).  Without knowing those things, without knowing the intended
behavior, the behavior could be nearly anything.

> So, rewrite the code as follows:
> 
> (defun my-beautiful-function ()
>     (save-current-buffer
>        (set-buffer B)
>        (goto-char x)
>        ...
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
>            (save-current-buffer
>               (set-buffer A)
>               (save-excursion
>                  (goto-char y)
>                  ...)))
> 
> Not only would you have mollified the compiler, but you will have much
> more robust code that will continue to function when used in
> unforeseen ways.

Oh, so you meant that `m-b-f' doesn't need to do anything specifically in A, and
it need not be called from A.  Such details were missing.  And `f-s' should not
move point in A (the buggy hypothesis).  In that case, sure, that's a perfectly
fine way to do what you apparently want.

It all depends on what the intended behavior is.  You might well want `f-s' to
move point in A.  You might, as part of its contract, know that `m-b-f' will
always be called from A, or from C, or whatever.

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

[Uday Reddy]

[This article that I sent to help-gnu-emacs@gnu.org on 13 Mar 2011, 
12:46:00, didn't show up on the news server.  However, Drew's response 
to it showed up on the news server.  Some lossage in the ether! I am 
posting it as a response to Drew's message, even though it preceded it.]


Tim X writes:

 > > Seems all what's neccessary to know about save-excursion already
 > > is expressed in it's docstring.
 > >
 >
 > Not sure thats true. I've seen a lot of elisp code that does the
 > save-excursion, set buffer pattern, which makes me think maybe the
 > documentation is inadequate.

It seems that reason for the widespread use of the
save-excursion/set-buffer pattern is actually quite simple.  Before
Emacs version 20.1, there was no other way to do it.
save-current-buffer was only introduced in 20.1.  So, there is a lot
of old code that uses the old pattern.  Now, there is also a lot of
new code that uses the old pattern because people learn their patterns
from the old code.

 > > If people don't want the buffer restored alongside with point and 
mark, they
 > > should not use this form.
 > >
 >
 > I think it is a little more subtle than that. People may be using it
 > under the expectation that point and mark will also be saved in the
 > buffer accessed by set-buffer.

No, I don't actually think that.  (I notice that Andreas has
questioned this assumption too.)

If at all people are doing it consciously, they do it because they
think it is harmless.  You want to preserve the current-buffer.  But,
if you also preserve the point and mark, it seems like harmless
redundancy.  So, what is the big deal?

It takes quite a bit of insight into the code to understand that it is
actually *harmful* redundancy.  The unintentional uses of
save-excursion cover up potential bugs inside the code which don't get
noticed because of this redundant saving.

So, my test is pure and simple.  Convert all the save-excursion's that
the compiler complains about to save-current-buffer.  If your code
continues to run perfectly, you are my hero.  If your code falls over,
then you should accept that your code is buggy and it is only able to
stand up on the crutches of what you thought was "harmless
redundancy".  Then it is up to you whether you want to fix the code or
continue to live with it.  But there are many of us who won't touch
such code with a tadpole because we regard it as buggy.

 > Except we know it is often used incorrectly. I think Uday's point is
 > very valid here. When writing code, part of the aim is to communicate
 > intentions to others who may need to maintain or update the code in the
 > future. As Uday pointed out, using save-excursion followed by set-buffer
 > muddies the intention and decreases clarity.

Unfortunately, it is more than a question of clarity.  It is a
question of correctness.  Let me restate my example from Friday a bit
more explicitly

(defun my-beautiful-function ()
     (save-excursion
        (set-buffer B)
        (goto-char x)
        (setq a (find-something))
        ...))

(defun find-something ()
         (....
            (set-buffer A)
            (goto-char y)
          ...))

find-something is *buggy* here.  It is moving point in buffer A though
it wasn't supposed to.  However, my-beautiful-function works fine as
long as I call it in buffer A because the "harmlessly redundant"
save-excursion at its top-level restores A's point.

Tomorrow, you decide to call my-beautiful-function from some other
buffer C, and all hell breaks loose.  You suddenly find that the point
is moving in the unrelated buffer A and you have no idea why.

So, rewrite the code as follows:

(defun my-beautiful-function ()
     (save-current-buffer
        (set-buffer B)
        (goto-char x)
        ...
        (setq a (find-something))
        ...))

(defun find-something ()
         (....
            (save-current-buffer
               (set-buffer A)
               (save-excursion
                  (goto-char y)
                  ...)))

Not only would you have mollified the compiler, but you will have much
more robust code that will continue to function when used in
unforeseen ways.

Cheers,
Uday

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

[Andreas Röhler]

Am 13.03.2011 13:46, schrieb Uday S Reddy:
> Tim X writes:
>
>>> Seems all what's neccessary to know about save-excursion already
>>> is expressed in it's docstring.
>>>
>>
>> Not sure thats true. I've seen a lot of elisp code that does the
>> save-excursion, set buffer pattern, which makes me think maybe the
>> documentation is inadequate.
>
> It seems that reason for the widespread use of the
> save-excursion/set-buffer pattern is actually quite simple.  Before
> Emacs version 20.1, there was no other way to do it.
> save-current-buffer was only introduced in 20.1.  So, there is a lot
> of old code that uses the old pattern.  Now, there is also a lot of
> new code that uses the old pattern because people learn their patterns
> from the old code.
>
>>> If people don't want the buffer restored alongside with point and mark, they
>>> should not use this form.
>>>
>>
>> I think it is a little more subtle than that. People may be using it
>> under the expectation that point and mark will also be saved in the
>> buffer accessed by set-buffer.
>
> No, I don't actually think that.  (I notice that Andreas has
> questioned this assumption too.)
>
> If at all people are doing it consciously, they do it because they
> think it is harmless.  You want to preserve the current-buffer.  But,
> if you also preserve the point and mark, it seems like harmless
> redundancy.  So, what is the big deal?
>
> It takes quite a bit of insight into the code to understand that it is
> actually *harmful* redundancy.  The unintentional uses of
> save-excursion cover up potential bugs inside the code which don't get
> noticed because of this redundant saving.
>
> So, my test is pure and simple.  Convert all the save-excursion's that
> the compiler complains about to save-current-buffer.  If your code
> continues to run perfectly, you are my hero.  If your code falls over,
> then you should accept that your code is buggy and it is only able to
> stand up on the crutches of what you thought was "harmless
> redundancy".  Then it is up to you whether you want to fix the code or
> continue to live with it.  But there are many of us who won't touch
> such code with a tadpole because we regard it as buggy.
>
>> Except we know it is often used incorrectly. I think Uday's point is
>> very valid here. When writing code, part of the aim is to communicate
>> intentions to others who may need to maintain or update the code in the
>> future. As Uday pointed out, using save-excursion followed by set-buffer
>> muddies the intention and decreases clarity.
>
> Unfortunately, it is more than a question of clarity.  It is a
> question of correctness.  Let me restate my example from Friday a bit
> more explicitly
>
> (defun my-beautiful-function ()
>      (save-excursion
>         (set-buffer B)
>         (goto-char x)
>         (setq a (find-something))
>         ...))
>
> (defun find-something ()
>          (....
>             (set-buffer A)
>             (goto-char y)
>           ...))
>
> find-something is *buggy* here.  It is moving point in buffer A though
> it wasn't supposed to.  However, my-beautiful-function works fine as
> long as I call it in buffer A because the "harmlessly redundant"
> save-excursion at its top-level restores A's point.
>
> Tomorrow, you decide to call my-beautiful-function from some other
> buffer C, and all hell breaks loose.  You suddenly find that the point
> is moving in the unrelated buffer A and you have no idea why.
>
> So, rewrite the code as follows:
>
> (defun my-beautiful-function ()
>      (save-current-buffer
>         (set-buffer B)
>         (goto-char x)
>         ...
>         (setq a (find-something))
>         ...))
>
> (defun find-something ()
>          (....
>             (save-current-buffer
>                (set-buffer A)
>                (save-excursion
>                   (goto-char y)
>                   ...)))
>
> Not only would you have mollified the compiler, but you will have much
> more robust code that will continue to function when used in
> unforeseen ways.
>
> Cheers,
> Uday
>
>


Hi Uday,

thanks trying so hard to explain your stand.

AFAIU you are undergoing some basic mistake. It's of the kind probably 
every one at this list already experienced it: getting things wrong 
which are trivial alltogether.

IMHO best method in these circumstances is changing the sujet, walking 
out, some relax.

Maybe let's have a break with that matter. Don't mix up VM :-)


Andreas

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

[Uday S Reddy]

Well, this discussion has gone haywire.  So, I would like to focus on
this example and leave out all the other shrubbery, hoping that this
might clarify the issue.  I am rewriting the text surrounding the
example, based on your questions.

This `my-beautiful-function' was designed to be called in a buffer A.

(defun my-beautiful-function ()
    (save-excursion
       (set-buffer B)
       (goto-char x)
       (setq a (find-something))
       ...))

(defun find-something ()
        (....
	   (save-excursion 
              (set-buffer A)  
              (goto-char y)
              ...)))

The assumption is that find-something is *buggy* here.  It is moving
point in buffer A though it wasn't supposed to.  (That is why I named
it "find-something" rather than "do-something" or "go-somewhere".)

The `save-excursion' at the outer level in `my-beautiful-function' was
presumably placed there to restore the current buffer.  Just looking
at its body, we don't see any point movements in buffer A.  So, we
might feel justified in thinking that the `save-excursion' can be
replaced by `save-current-buffer'.  But let us leave that aside for
the moment.

As long as you use `my-beautiful-function' in buffer A, it will appear
to work fine, because the unprotected point movement in
`find-something' is cancelled by the `save-excursion' at the
top-level.  If you call it in some other buffer C, it will
unexpectedly move point in buffer A.

My position is that `find-something' is *inherently buggy* because of
its unprotected point movement in buffer A.  The fact that it appears
to work fine as long as it is called from `my-beautiful-function'
invoked in buffer A, doesn't make it right.

The `save-excursion' at the top-level of `my-beautiful-function'
serves to cover up the bug, whether intentionally or unintentionally.  

The solution is to change the `save-excursion' to
`save-current-buffer', exposing the bug, and then to track it down.

> Tell us more.  What buffer is `m-b-f' called in?  Presumably, if it
> is intended to be called in buffer A, then it wants to restore point
> in A when it is done (assuming the `save-excursion' is the last
> thing it does).

Why do you think m-b-f "wants" to restore point in A, and not in other
buffers?  What makes the buffer A special?  Why is it that you should
always restore the point in the current buffer and leave all the other
buffers to their fate?

> Presumably, if it is intended instead to be called in buffer C then
> it wants to restore point in C when it is done.  In this case,
> presumably (if we don't just assume that it is buggy)
> `find-something' wants to leave point in A at Y (assuming it doesn't
> move it in the last `...').

It may not may have been intended to be called in buffer C, but
nothing in the design might indicate that it is intended to be called
only in buffer A.  So, you would be justified in calling it in
another buffer.

> There is no way to know from your skeleton what the purpose or the
> behavior is.  The devil is in the details...which are missing.

So, how much detail would one need to see before one can infer basic
facts about point movements and buffer changes?  

> There is nothing intrinsically wrong with a function that moves point to some
> position in some buffer and leaves it there.  I probably wouldn't name such a
> function just `find-something', however, and I would document that part of its
> purpose is to move point in buffer A or whatever.  Assuming that is the case.

I am sure you you would word things better.  But we live in a free
software world where lots of people contribute code.  The wording may
not be clear, the names may not be clear, and there may be no docs
whatsoever.  You have to infer what you want to infer by looking at
the code.

> > However, my-beautiful-function works fine as
> > long as I call it in buffer A because the "harmlessly redundant"
> > save-excursion at its top-level restores A's point.
> 
> Oh, so you were presuming that `m-b-f' wants to restore A's point.
> OK.  Where's the problem?  (So far nothing is _redundant_, however.)

I have no idea what it means for m-b-f "wanting" to restore A's point.
I don't see why it should care about the buffer A at all.  The way I
view things, m-b-f should restore whatever temporary point movements
it makes.  It shouldn't be covering up the bugs in other functions
like `find-something'.  But the way the save-excursion/set-buffer
combination works, everything is covering up everything else's bugs.
The code might be completely infested with bugs.  But we will never
know, because it is also constantly covering them up!

> Or if by hypothesis `f-s' is buggy and should not in fact move point
> in A, then you should be able to see by debugging that it in fact
> does move point in A, even when `m-b-f' is called in A.  And you
> should be able to tell just by looking at the code that it moves
> point in A.
> 
> I don't deny that if the overall code is complex such a bug might
> not be obvious, but I submit that this is a general problem that has
> nothing special to do with `save-excursion' or `set-buffer'.  It is
> simply about keeping track of the side effects performed by your
> code.
> 
> `find-something' as written has the side effect of moving point in
> buffer A - always.  If that is not the intention, then yes, its bug
> will not surface as long as it is called inside a `save-excursion'
> for buffer A.

Great, I seem to have hit a home run.  `find-something' bug will not
surface as long as it is called inside a `save-excursion' for buffer
A.  You said it!

Now, what plausible reason is there for m-b-f to use `save-excursion'
if not for covering up the `find-something' bug?  If the bug wasn't
there, we can simply convert `save-excursion' to `save-current-buffer'
and we will be fine.

My belief is that all save-excursions used with set-buffer are
precisely there to cover up bugs.  If there is any other conceivable
reason why `save-excursion' should be used with `set-buffer', then
please show me!

Cheers,
Uday

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

[Stefan Monnier]

> Ok, how about
>    "save-current-buffer is a better choice than save-excursion"

Hmm, indeed, maybe the message should simply say

  "Use with-current-buffer instead of save-excursion+set-buffer"

> I only wish that the compiler warning was there 20 years ago so that
> I wouldn't have to deal with that mess now.

Oh yes!


        Stefan

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

[Stefan Monnier]

> So, my test is pure and simple.  Convert all the save-excursion's that
> the compiler complains about to save-current-buffer.  If your code

I tend to push with-current-buffer rather than save-current-buffer.
The idea is that for most coding purposes, you only need save-excursion
to save point&mark and with-current-buffer to move to a another buffer
(and set-buffer is basically never needed).

I actually do C-s (set-buffer fairly often, as part of random code cleanup.


        Stefan

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

[Uday Reddy]

On 3/14/2011 2:18 PM, Andreas Röhler wrote:
>
> Maybe let's have a break with that matter. Don't mix up VM :-)

Oh, no, I don't intend to give the impression that VM is mixed up in any 
way.  Almost all the code written before the present compiler warning 
was using the save-excursion/set-buffer combination.  It is in inherent 
in that combination that it covers up bugs involving unprotected point 
movements.

The only thing special about VM is that it deals with multiple buffers 
and switches between them a lot.  So, it is incredibly hard to track 
down the unprotected point movements.  For all I know, there could be a 
couple of problems.  But it will be a lot of hard work to find them.

Cheers,
Uday

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

[Andreas Röhler]

Am 14.03.2011 15:25, schrieb Stefan Monnier:
>> Ok, how about
>>     "save-current-buffer is a better choice than save-excursion"
>
> Hmm, indeed, maybe the message should simply say
>
>    "Use with-current-buffer instead of save-excursion+set-buffer"
>
>> I only wish that the compiler warning was there 20 years ago so that
>> I wouldn't have to deal with that mess now.
>
> Oh yes!
>
>
>          Stefan
>

Hi Stefan,

pointing to other ways of delivering a task doesn't prove

save-excursion+set-buffer

is wrong.

Nonewithstanding there are better resp. lesser good coding styles.
Always welcome a discussion of code quality.

As far it concerns the compiler, please have a look how many warnings 
the last release issues when compiling.

In the result people will ignore that plenty of warnings.
Or might say: thats not worth a release if so many bugs are left.

That's going bad that way.

Whilst Emacs 24 alltogether seems coming forward considerably and is 
used in daily work here - thanks BTW.

Andreas

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

[Drew Adams]

You repeated a lot Uday, but that's OK if it helps you understand or express
yourself.

> I am rewriting the text surrounding the example, based on your questions.
> This `my-beautiful-function' was designed to be called in a buffer A.
> 
> (defun my-beautiful-function ()
>     (save-excursion
>        (set-buffer B)
>        (goto-char x)
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
> 	   (save-excursion 
>               (set-buffer A)  
>               (goto-char y)
>               ...)))
> 
> The assumption is that find-something is *buggy* here.  It is moving
> point in buffer A though it wasn't supposed to.

Though you did not state this as part of your scenario, since you have mentioned
it otherwise may I add the assumption that this hypothetical code came from an
existing application written by a good programmer, and that that code generally
works well and has done so for quite some time?

IOW, I think you are trying to point out that code that has worked well can
nevertheless be fragile.  So let's assume that with no changes the code works,
and the person who wrote it was no fool - s?he wrote what s?he intended and
needed.

Let's imagine further that this code was inherited by someone else, and that
s?he is not necessarily familiar with all of the details.

> The `save-excursion' at the outer level in `my-beautiful-function' was
> presumably placed there to restore the current buffer.

Given the assumptions that I added (fairly, I hope), I would say that presumably
the `save-excursion' is there NOT ONLY to restore which buffer was current but
also restore point and mark in that buffer.  That's what `save-excursion' DOES,
and there is no reason not to suppose that the original author did not intend
that.

Even if the inheritor is not sure of things, we must give the benefit of the
doubt to the original designer/programmer, if for no other reason than the code
works.

IOW, given inherited code that seems to work, assume as a start that it works
_because_ of the code that is there.  Do not assume that the code can just be
altered willy-nilly, replacing some functionality by a subset that does not do
the whole job.  That might be the case, and perhaps the inheritor can improve
the code, but it would be a mistake to just assume that things can be changed
willy-nilly.

> Just looking at its body, we don't see any point movements in buffer A.
> So, we might feel justified in thinking that the `save-excursion' can be
> replaced by `save-current-buffer'.  But let us leave that aside for
> the moment.

I think that would be a big mistake - a very rash assumption.  A wiser and more
conservative assumption would be that even if you see no A-buffer point
movements directly in `m-b-f', somewhere in some code invoked within the
`save-excursion' there are or probably are A-buffer movements.  In fact,
precisely because you do not see point-A movements in `m-b-f' itself, you can be
almost sure there could be such in code called withing the `save-excursion'.
Why?  Because the original programmer used `save-excursion'.

IOW, again, do NOT assume that the `save-excursion' is intending to only
`save-current-buffer'.  Such an assumption is unwarranted based only on a glance
at the `m-b-f' code - after all, it does call other code.

> As long as you use `my-beautiful-function' in buffer A, it will appear
> to work fine, because the unprotected point movement in
> `find-something' is cancelled by the `save-excursion' at the
> top-level.  If you call it in some other buffer C, it will
> unexpectedly move point in buffer A.

Correct.

> My position is that `find-something' is *inherently buggy* because of
> its unprotected point movement in buffer A.

Well of course that's your position - and mine too, since you _posited_ that as
your hypothesis!  You seem to be trying to conclude what you hypothesized. (??)

> The fact that it appears to work fine as long as it is called
> from `my-beautiful-function' invoked in buffer A, doesn't make it right.

Dunno about "right".  But it works (by hypothesis).

Again, I think you are saying that code that works can nevertheless be fragile.
I might add: especially if the code is maintained by someone other than the
original author - or even, as sometimes happens to me, if the original author
(me) has simply forgotten some of the logic/details. ;-)

> The `save-excursion' at the top-level of `my-beautiful-function'
> serves to cover up the bug, whether intentionally or unintentionally.

Correct.  The original programmer's use of `save-excursion' does what s?he
wants: ensure that buffer A and its point and mark are restored, regardless of
what the code in the body of the `save-excursion' might do.  IOW, the `m-b-f'
code _ensures_ against any A-buffer point movements, whether from bugs or
otherwise.

The code in `f-s' is buggy by hypothesis, since you said that it was not
intended that it move point in A, but even if it were not buggy and that
movement were intentional, if the intent of using the `save-excursion' is what
we must assume it is (restore point as well as the buffer), then things work as
they should.  Call it a "cover up" if you want, but that's the intention behind
and the documented behavior of `save-excursion'.

> The solution is to change the `save-excursion' to
> `save-current-buffer', exposing the bug, and then to track it down.

That would not be my solution, but feel free.  Rather, that might be one way to
help you find the bug, if it really need be found.  But it is also problematic -
be sure to remember to return to using `save-excursion' if needed.

I would probably let the sleeping dog lie, and wait until the `f-s' bug actually
manifested itself.  After all, the `save-excursion' code does just what you want
here (by hypothesis): it ensures that _even if_ some code in the body (rightly
or wrongly) moves point in A, point in A will get restored.

If you later call `m-b-f' from some other buffer (e.g. Z) than A, then hopefully
you would ask yourself this immediately:

 "Hey, this `save-excursion' restores things for the buffer
 that was current before.  What buffer was that?  A.  So if I
 now call it from buffer Z then I probably need to wrap things
 in `with-current-buffer' or some such.  I certainly cannot just
 assume that now I should restore things in Z instead of A."

Or else, if you do not want to just wrap that way, you will want to analyze
things in detail to find out just what point movements are involved, why the
code restored point in A, etc.

Or if you do not reflect this way, hopefully you will see some change in
behavior and track down the bug in `f-s'.

The point is that you should _not_ just _assume_ that `save-excursion' is used
only in a way that treats it as `save-current-buffer'.  Assume instead that all
of its behavior is needed, then work from there.

You can, as you suggest, try substituting `save-current-buffer' to see what
happens (i.e. as a diagnostic exercise), but be prepared for (look out for)
behavior changes.

> > Tell us more.  What buffer is `m-b-f' called in?  Presumably, if it
> > is intended to be called in buffer A, then it wants to restore point
> > in A when it is done (assuming the `save-excursion' is the last
> > thing it does).
> 
> Why do you think m-b-f "wants" to restore point in A, and not in other
> buffers?

Because the original programmer said so, a priori, by choosing to use
`save-excursion' in buffer A.  That's what `save-excursion' does.  We must
presume that the programmer knew what s?he was doing and knew that point in A
needed to be restored.

On what basis could you presume anything different?  You seem to be so convinced
(unlike Stefan, BTW) that `save-excursion' is evil that you assume that its
point-saving behavior is not needed (in code that works).

> What makes the buffer A special?  Why is it that you should
> always restore the point in the current buffer and leave all the other
> buffers to their fate?

That is what `save-excursion' does.  If you want something different, then code
something different.  By hypothesis, the original programmer used
`save-excursion', and knew what s?he was doing.  Don't ask me why s?he felt that
A needed restoring and not other buffers.  But s?he did (by hypothesis), or s?he
wouldn't have used `save-excursion'.

> > Presumably, if it is intended instead to be called in buffer C then
> > it wants to restore point in C when it is done.  In this case,
> > presumably (if we don't just assume that it is buggy)
> > `find-something' wants to leave point in A at Y (assuming it doesn't
> > move it in the last `...').
> 
> It may not may have been intended to be called in buffer C, but
> nothing in the design might indicate that it is intended to be called
> only in buffer A.  So, you would be justified in calling it in
> another buffer.

Ooops.  No, but thanks for playing.  Something in the (inherited) implementation
is crying loud and clear that it is intended to be called from A and it is
intended to restore A.  What is that?  `save-excursion' was called from buffer
A, and its job is to restore the current buffer.

This couldn't be clearer.  You ignore such a clear indication of programmer
intention (implementation and probably design) at your own peril.

Let me repeat: `save-excursion' is _not_ `save-current-buffer'.  You substitute
the latter for the former willy-nilly at your own peril.

> > There is no way to know from your skeleton what the purpose or the
> > behavior is.  The devil is in the details...which are missing.
> 
> So, how much detail would one need to see before one can infer basic
> facts about point movements and buffer changes?

See above.  Infer from the presence of `save-excursion' that there is a _need_
to restore the original buffer, which was A, as well as its point and mark.

That is your best assumption, since the code has been working and the programmer
was no idiot.  Assume anything else by way of shortcut at your own peril.  Tread
carefully and analyze closely.

Again, it's fine to try substituting `save-current-buffer' as a probing
operation, to try to see what will happen - what might break, but be aware that
such a switch automatically changes the behavior: you are no longer saving and
restoring point (and mark).

> > > However, my-beautiful-function works fine as
> > > long as I call it in buffer A because the "harmlessly redundant"
> > > save-excursion at its top-level restores A's point.
> > 
> > Oh, so you were presuming that `m-b-f' wants to restore A's point.
> > OK.  Where's the problem?  (So far nothing is _redundant_, however.)
> 
> I have no idea what it means for m-b-f "wanting" to restore A's point.

The presence of `save-excursion' indicates that the current buffer's point will
be restored.  By hypothesis the code reflects the programmer's intention (it
works and has been used for a long, long time).

`save-excursion' "wants" to restore point, and it does so.

> I don't see why it should care about the buffer A at all.  The way I
> view things, m-b-f should restore whatever temporary point movements
> it makes.

Well you are jumping to a wild conclusion then, one not based on the existing
code.  The existing code says only that `m-b-f' should restore point for the
original buffer (which is A).

If the intent had been to restore point movements in all buffers then a very
different program would have to have been written.  `save-excursion' does NOT do
that.  One of Stefan's points (and a/the reason for the warning) was that some
people can get fooled into thinking it does, but at this point at least _you_
should not be so fooled.

Nothing in that code indicates any intention to restore point movements in all
buffers.  Absolutely nothing.  Quite the contrary.  The code is telling us loud
and clear that the intention is to restore point _only_ in the original buffer
(e.g., A).

> It shouldn't be covering up the bugs in other functions
> like `find-something'.  But the way the save-excursion/set-buffer
> combination works, everything is covering up everything else's bugs.
> The code might be completely infested with bugs.  But we will never
> know, because it is also constantly covering them up!

Dunno what to say, Uday, without repeating myself more.  `save-excursion' saves
and restores point in only one buffer.  If that is not the intention then don't
use it.  What you call "covering up" is exactly the ignoring of temporary point
movements (for _one_ buffer).

> > `find-something' as written has the side effect of moving point in
> > buffer A - always.  If that is not the intention, then yes, its bug
> > will not surface as long as it is called inside a `save-excursion'
> > for buffer A.
> 
> Great, I seem to have hit a home run.  `find-something' bug will not
> surface as long as it is called inside a `save-excursion' for buffer
> A.  You said it!

Glad you got it.  Whether buggy or intended, any A-buffer point movements in
`f-s' are ultimately lost if it is called within a `save-excursion' invoked from
A.  That's the intention behind `save-excursion'.

> Now, what plausible reason is there for m-b-f to use `save-excursion'
> if not for covering up the `find-something' bug?

Uh, to do what it does?  To ignore point movements in the buffer it's called
from?  We have to presume that it _means_ to do that, unless you have some
reason not to.

> If the bug wasn't there, we can simply convert `save-excursion'
> to `save-current-buffer' and we will be fine.

If `save-current-buffer' were all that was needed here to start with, that is,
if the programmer did _not_ intend/need to restore point for buffer A, then
`save-excursion' would not have been used.  Even in code written long, long ago
the programmer would have simply saved and restored the buffer and not also the
values of point and mark.

> My belief is that all save-excursions used with set-buffer are
> precisely there to cover up bugs.

What can I say, Uday?

> If there is any other conceivable reason why `save-excursion' should
> be used with `set-buffer', then please show me!

Please read the original thread if you don't get it by now.

What if you need to save and later restore point in a buffer, Uday, what do
_you_ do?

You can use `save-excursion' with `save-current-buffer' or with
`with-current-buffer' instead of with `set-buffer', but you still need to use
`save-excursion' (or roll your own point-saving/restoring code).

Your beef seems to be with `save-excursion' and the fact that it saves and
restores point for a buffer.  Your beef _should_ be with `set-buffer', I would
think.  I would think you would simply be arguing for using 1 instead of 2, in
general:

1. (save-excursion (with-current-buffer Z...))
2. (save-excursion (set-buffer Z)...)

But #1, like #2 has all of the bad "cover-up" behavior you decry.
`save-excursion' saves point for the buffer in which it is invoked.  Invoke it
from a different buffer with no other changes and you are likely to get some
behavior you don't want.

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

[Juanma Barranquero]

On Sat, Mar 12, 2011 at 20:56, Drew Adams <drew.adams@oracle.com> wrote:

> IMO, this warning has produced _far_ more confusion than it has eliminated.

That's bad, because it would be better if the warning eliminated
confusion too; but it is secondary, because its true purpose is to
eliminate subtle bugs, which Stefan already explained like twenty
times (soon he will be able to write a book à la Italo Calvino with
all this). So even if some not-really-understanding elisp programmer
just reacts to the warning and changes his code, he wil already reach
some benefit.

From my point of view, your arguments support fixing the wording, not
removing the warning.

> FWIW, I also agree with Andreas that a "warning" is for something serious.  A
> warning is not the same thing as in informative message.  A warning _warns_ you
> about potential danger/damage.

Curious. In computing terminology, I'd say an *error* is for something
serious. A warning informs you about a potential problem, and often
one that you're likely to miss by yourself. Just like this one.

IMHO, 0.02€, etc. etc.

    Juanma

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

[Drew Adams]

> > FWIW, I also agree with Andreas that a "warning" is for 
> > something serious.  A warning is not the same thing as in
> > informative message.  A warning _warns_ you about potential
> > danger/damage.
> 
> Curious. In computing terminology, I'd say an *error* is for something
> serious. A warning informs you about a potential problem, and often
> one that you're likely to miss by yourself. Just like this one.

Problem, yes, and typically a serious one.  Not like the information in this
message, IMO.

An error message is typically for something that has already happened (or is
happening now).  A warning message typically warns you about something serious
that could happen (potential, future).

By way of example, in one computing company the documentation style guide says
this about a Warning notice:

 4.6.3 Warning Notices
 Use a Warning notice to call attention to a situation that is
 potentially hazardous to people. The warning notice should
 always precede the step where the risk occurs, and the text
 should always be in boldface font.

 Warning:

 To reduce risk of electrocution, unplug the machine before you
 begin repairs.

Note the emphasis on danger to a _person_, at least for this particular style
guide.  The same guide says this about a Caution notice:

 4.6.2 Caution Notices
 Use a Caution notice to indicate the possibility of damage
 to a program, system, or data. The caution notice should
 always precede the step where the risk occurs.

 Caution:

 Always use ________ to bring ________ online and offline.
 Using other tools can result in unintended _____ failover
 or jeopardize the integrity of your _______.

And it says this about informative Notes:

 4.6.1 Notes
 Use Note to call attention to points that might easily be
 overlooked. A note includes important hints, tips, guidance,
 or advice on using a program. It also includes information
 that is essential to the proper operation of a program or
 system, provided that the information does not belong in
 a Caution or Warning notice.

 Note:

 The installation stops and the system returns an error if
 the prerequisite software is not installed.

 Note to UNIX Users:

 This feature is not available for the UNIX operating system
 in the Beta software. Please install the current release
 before using this feature.

Those concern doc notices, not messages, but similar considerations apply.  Some
organizations do not distinguish Warning from Caution, but the main idea is that
these things warn you about a serious potential problem.  They do not simply
provide important guidance or essential information for proper use.  

HTH.

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

[Uday S Reddy]

Drew Adams writes:

> Though you did not state this as part of your scenario, since you
> have mentioned it otherwise may I add the assumption that this
> hypothetical code came from an existing application written by a
> good programmer, and that that code generally works well and has
> done so for quite some time?

No, it wasn't from an actual application.  I just made it up.  And, it
isn't good code either.  To make it good code, one needs another
save-excursion to protect the point movement in buffer A, as below:

     (defun find-something ()
        (....
	   (save-excursion 
              (set-buffer A)  
	      (save-excursion
                 (goto-char y)  
                  ...))))

The outer save-excursion essentially plays the role of
save-current-buffer and it is mostly harmless when used in that way.
The second save-excursion is needed to reverse the excursion of the
point to y.  Without the second save-excursion, I would consider the
code buggy.

Once that bug is fixed, the remaining save-excursions (the ones at the
top-level in `find-something' as well as `my-beautiful-function') can
be safely converted to save-current-buffer.  And, the code will work
fine.  This is what I mean by tracking down the bugs covered up by
redundant save-excursions.  The clean-up procedure involves:

- Converting a few save-excursions flagged up by the compiler to
save-current-buffer.

- Testing to see if those conversions break anything.

- If they do, then hunt down the unprotected excursions that have been
exposed in the process and wrap them in save-excursions.

And repeat.  It is painstaking work.

You might wonder why do this at all.  Why not just leave the code as
it is since it is working fine?  That is a judgement call for you as
the developer.  If it is important code and you expect it to get used
for a long time hence, it pays to clean it up, because you never know
when those unprotected excursions will come back to bite you.  

> IOW, I think you are trying to point out that code that has worked
> well can nevertheless be fragile.  So let's assume that with no
> changes the code works, and the person who wrote it was no fool -
> s?he wrote what s?he intended and needed.

If that code was found in an actual application, then the only thing
that can be said in defence of the original developer is that it must
have been an oversight.  In the pre-20.1 elisp, save-excursion was the
only primitive available to restore the current buffer, which had the
unfortunate effect of covering up all the unprotected excursions.  So,
s?he didn't notice the problem.  If s?he deliberately wrote such code,
then we should probably dump it and go find something else to work on.

> Given the assumptions that I added (fairly, I hope), I would say
> that presumably the `save-excursion' is there NOT ONLY to restore
> which buffer was current but also restore point and mark in that
> buffer.  That's what `save-excursion' DOES, and there is no reason
> not to suppose that the original author did not intend that.

No way!  If the original author deliberately used the outer
save-excursion to cover up unprotected excursions, then the code is
really bad.  Let me amplify that point because this seems to be the
crux of the discussion:

   (save-excursion
      (set-buffer B)
      ....
      (setq a (find-something))
      ...)

The outer save-excursion is restoring the current buffer's point and
mark.  There is nothing visible in the body that does anything to the
current buffer's point and mark.  So, the ONLY reason the outer
save-excursion needs to restore the current buffer's point and mark is
the expectation that `find-something' is going to do something cheeky.
How can you trust such a programmer?

> IOW, given inherited code that seems to work, assume as a start that
> it works _because_ of the code that is there.  Do not assume that
> the code can just be altered willy-nilly, replacing some
> functionality by a subset that does not do the whole job.  That
> might be the case, and perhaps the inheritor can improve the code,
> but it would be a mistake to just assume that things can be changed
> willy-nilly.

If the code doesn't allow me to alter it "willy-nilly" then it isn't
good code.  Of course, I would need to understand its function, the
data structures and invariants.  But, assuming I have understood all
that, I should be able to look at pieces of code locally and modify
them.  

> IOW, again, do NOT assume that the `save-excursion' is intending to
> only `save-current-buffer'.  Such an assumption is unwarranted based
> only on a glance at the `m-b-f' code - after all, it does call other
> code.

I think I have already made my disagrement plain.  It is a bad idea
for `my-beautiful-function' to be negating the other code's point
movements at the top level.  If it is clear from the way the "other
code" is put together that its purpose is to change buffer and move
point, and m-b-f is deliberately calling that code to achieve those
effects, then why is it reversing all those changes via a
save-excursion?  That doesn't make much sense.  Either

- the other code is being called for its effects, in which case we
have no need to reverse them, or

- the other code is being called for some other results, in which case
it should be cleaning up after itself.

Slapping a save-excursion at the top-level, which deceptively looks
like a save-current-buffer, to do a brute force clean-up after the
fact is not a very clever way of arranging things.

> On what basis could you presume anything different?  You seem to be
> so convinced (unlike Stefan, BTW) that `save-excursion' is evil that
> you assume that its point-saving behavior is not needed (in code
> that works).

No, I didn't say that.  `save-excursion' is definitely used to save
the point and mark.  But when used for that purpose, it is placed
where the point or mark are being moved.  If it is used to save the
buffer, it is placed in front of a set-buffer.  I have never really
seen it being used to do both.  If I have to imagine how it could be
used to do both, it would be something like this:

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

A second save-excursion would be needed after switching to B, because
whatever is done afterwards is *not* going to be protected by the
original save-excursion.

> > What makes the buffer A special?  Why is it that you should
> > always restore the point in the current buffer and leave all the other
> > buffers to their fate?
> 
> That is what `save-excursion' does.  If you want something
> different, then code something different.  By hypothesis, the
> original programmer used `save-excursion', and knew what s?he was
> doing.  Don't ask me why s?he felt that A needed restoring and not
> other buffers.  But s?he did (by hypothesis), or s?he wouldn't have
> used `save-excursion'.

We shouldn't call `save-excursion' willy-nilly just because it
exists.  If the buffer A is indeed special and we should restore only
its excursions but not those of other buffers, then the right place to
protect them is wherever the excursions are happening, not at the
top-level.  

Morever, what if that is not what we want?  What if we want to protect
the excursions in all the buffers, not only the current-buffer of
`m-b-f'?  The top-level `save-excursion' is not going to help you
achieve that.  What do you do then?

On a closer look, this kind of programming doesn't look well-thought
out at all.  It seems quite arbitrary.

> Let me repeat: `save-excursion' is _not_ `save-current-buffer'.  You
> substitute the latter for the former willy-nilly at your own peril.

I know.  I was writing to Stefan earlier today saying exactly that.
`save-excursion' should be replaced by `save-current-buffer' only
after ensuring that all excursions are properly protected.  I have
described my clean-up procedure earlier in the message.

> Dunno what to say, Uday, without repeating myself more.
> `save-excursion' saves and restores point in only one buffer.  If
> that is not the intention then don't use it.  What you call
> "covering up" is exactly the ignoring of temporary point movements
> (for _one_ buffer).

"Only one buffer" is a mistaken assumption.  Typically, in this kind
of code, there would be save-excursions all over the place.  Whenever
point movements happen, they would be under a thick stack of
save-excursions for all kinds of buffers.  The buffer you are doing
excursions in will typically end up being in that stack accidentaly
and so its excursion will get cancelled before the whole process ends.
Somehow magically the code works, even though you don't know how.
When you tweak the code a little, some buffer might get eliminated
from the stack and, all of a sudden, its excursions will get exposed
to the user.  "No problem!" says the hacker.  Slap on another
save-excursion, and the code starts to work magically again.

> > If there is any other conceivable reason why `save-excursion' should
> > be used with `set-buffer', then please show me!
> 
> Please read the original thread if you don't get it by now.
> 
> What if you need to save and later restore point in a buffer, Uday, what do
> _you_ do?

What do _I_ do?  I never need to save and restore the point in a
buffer.  I always place my save-excursions next to the excursions.
Only those point movements that are supposed to be the inteded effects
of the operations will be outside any save-excursion.  If I make any
mistakes in doing this, I go find them and fix them.  So, I don't need
any save-excursions at the top-level.  The code that I maintain works
exactly in that way.  And that is what the Elisp manual says
you should do as well:

       It is often useful to move point "temporarily" within a
       localized portion of the program, or to switch buffers
       temporarily.  This is called an "excursion", and it is done
       with the `save-excursion' special form.

Cheers,
Uday

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

[Drew Adams]

> one needs another save-excursion
> to protect the point movement in buffer A, as below:
>      (defun find-something ()
>         (....
> 	   (save-excursion 
>               (set-buffer A)  
> 	      (save-excursion
>                  (goto-char y)  
>                   ...))))

If you say so.  It all depends on what the code is trying to do.  You don't seem
to get the fact that not all point movement is bad and needs to be "protected".

> Without the second save-excursion, I would consider the code buggy.

And what if the function in question has as part of its contract to move point
in buffer A to Y?  Everything depends on what the desired behavior is.

`find-something' would deserve a better name in that case (it already deserves
one), but in that case the poor programming practice would amount to just poor
naming, not a lack of a second `save-excursion' to "protect" point movement in
A.

> Once that bug is fixed, the remaining save-excursions (the ones at the
> top-level in `find-something' as well as `my-beautiful-function') can
> be safely converted to save-current-buffer.  And, the code will work
> fine.  This is what I mean by tracking down the bugs covered up by
> redundant save-excursions.  The clean-up procedure involves:
> 
> - Converting a few save-excursions flagged up by the compiler to
>   save-current-buffer.
> - Testing to see if those conversions break anything.
> - If they do, then hunt down the unprotected excursions that have been
>   exposed in the process and wrap them in save-excursions.
> 
> And repeat.  It is painstaking work.

But you have the mission and motivation to do it, apparently.
OK by me, if that's what you want to do.  What's the problem?

> You might wonder why do this at all.

It depends on what behavior is desired/correct.  Maybe you've done the right
thing, maybe not.  Certainly you cannot generalize from such a case.  Other
functions will have different intentions and different needs.

> Why not just leave the code as it is since it is working fine?

Oh, was it working fine?  I thought you had to fix it.

> That is a judgement call for you as the developer.

Couldn't agree more.

> > IOW, I think you are trying to point out that code that has worked
> > well can nevertheless be fragile.  So let's assume that with no
> > changes the code works, and the person who wrote it was no fool -
> > s?he wrote what s?he intended and needed.
> 
> If that code was found in an actual application, then the only thing
> that can be said in defence of the original developer is that it must
> have been an oversight.

Why?  Because you think `save-excursion' is evil?

> In the pre-20.1 elisp, save-excursion was the
> only primitive available to restore the current buffer, 

You don't need a primitive to do that.

> which had the unfortunate effect of covering up all the
> unprotected excursions.

Nothing unfortunate about it.  The _aim_ of `save-excursion' includes saving and
restoring the buffer's point and mark.  You don't seem to get that, so you go
round and round.

If you don't want to save and restore point, then hey, don't use
`save-excursion'.  End of story.

And back in Emacs 18 you didn't need to wait until `save-current-buffer' was
introduced to save and restore only the current buffer.  You should be able to
do that with just an `unwind-protect' and a `let'.  (No, I haven't looked at the
C code to see what `save-current-buffer' really does.)

> > presumably the `save-excursion' is there NOT ONLY to restore
> > which buffer was current but also restore point and mark in that
> > buffer.  That's what `save-excursion' DOES, and there is no reason
> > not to suppose that the original author did not intend that.
> 
> No way!  If the original author deliberately used the outer
> save-excursion to cover up unprotected excursions, then the code is
> really bad.  Let me amplify that point because this seems to be the
> crux of the discussion:
> 
>    (save-excursion
>       (set-buffer B)
>       ....
>       (setq a (find-something))
>       ...)
> 
> The outer save-excursion is restoring the current buffer's point and
> mark.  There is nothing visible in the body that does anything to the
> current buffer's point and mark.  So, the ONLY reason the outer
> save-excursion needs to restore the current buffer's point and mark is
> the expectation that `find-something' is going to do something cheeky.

Cheeky?

> How can you trust such a programmer?

Why wouldn't you trust such a programmer?

What's special about "visible in the body"?  What if your `find-something' is
instead (goto-char (point-min)) or (beginning-of-buffer)?  What does that
change?  Does that make you feel better?

OK, that's too easy for you.  What if it was (my-goto-beginning-of-buffer)?  Now
you can guess what it does - likely similar to (goto-char (point-min)).
Suddenly your rotten, untrustworthy programmer is pretty trustworthy.  Or at
least I would hope you would find her so.

Your problem seems to be about visibility.  No one is arguing that code cannot
become complex, or even be simple but with poor naming, and so be difficult to
understand.  That has nothing to do with whether the code you show here is bad.

> > IOW, given inherited code that seems to work, assume as a start that
> > it works _because_ of the code that is there.  Do not assume that
> > the code can just be altered willy-nilly, replacing some
> > functionality by a subset that does not do the whole job.  That
> > might be the case, and perhaps the inheritor can improve the code,
> > but it would be a mistake to just assume that things can be changed
> > willy-nilly.
> 
> If the code doesn't allow me to alter it "willy-nilly" then it isn't
> good code.

OK.  You stick to your near global-replace and you'll be fine.

> Of course, I would need to understand its function, the
> data structures and invariants.  But, assuming I have understood all
> that, I should be able to look at pieces of code locally and modify
> them.
> 
> > IOW, again, do NOT assume that the `save-excursion' is intending to
> > only `save-current-buffer'.  Such an assumption is unwarranted based
> > only on a glance at the `m-b-f' code - after all, it does call other
> > code.
> 
> I think I have already made my disagrement plain.  It is a bad idea
> for `my-beautiful-function' to be negating the other code's point
> movements at the top level.

You apparently have the same gripe about `save-excursion', quite simply: you
don't think `save-excursion' should save and restore point.  Replace `m-b-f' by
`save-excursion' in your overall argument and it stays essentially the same.
No?

> If it is clear from the way the "other code" is put together
> that its purpose is to change buffer and move point,
> and m-b-f is deliberately calling that code to achieve those
> effects, then why is it reversing all those changes via a
> save-excursion?

Reuse.  The called code might have more behavior as its general purpose than
just changing point.  If `m-b-f' doesn't care about that point movement and
decides to ignore (undo) it, that's perfectly fine.  Perfectly fine, I said.

>  That doesn't make much sense.  Either
> - the other code is being called for its effects, in which case we
>   have no need to reverse them, or

Moving point is not the only possible effect.  And it might be called not only
for its effects but also for its return value.  There are many possible reasons
why it might be worthwhile to call a function that also moves point, even if one
of the callers doesn't really care about that point movement and chooses to
ignore (undo) it.

> - the other code is being called for some other results, in which case
> it should be cleaning up after itself.

Why?  As far as it is concerned, moving point is part of what it does,
purposefully.  That doesn't mean that every caller needs to care about that
movement.  Neither caller nor callee are at fault here.  No fault.  No problem.

> Slapping a save-excursion at the top-level, which deceptively looks
> like a save-current-buffer,

"Deceptively looks like a `save-current-buffer'"?  Do you think that
`save-excursion' is somehow out to get you?  Are you worried that
`save-excursion' is intentionally trying to trick Uday?

> to do a brute force clean-up

Brute force?  Restoring point is a brute force operation?  Is `save-excursion' a
terrorist operation?  Rambo?

> after the fact is not a very clever way of arranging things.

No comment.

> > On what basis could you presume anything different?  You seem to be
> > so convinced (unlike Stefan, BTW) that `save-excursion' is evil that
> > you assume that its point-saving behavior is not needed (in code
> > that works).
> 
> No, I didn't say that.  `save-excursion' is definitely used to save
> the point and mark.  But when used for that purpose, it is placed
> where the point or mark are being moved.  If it is used to save the
> buffer, it is placed in front of a set-buffer.  I have never really
> seen it being used to do both.

I suggest you look more closely at the zillion lines of code you inherited.  You
might find a few examples where `save-excursion' is used precisely for what it
does: restore the buffer, its point and its mark.

> If I have to imagine how it could be
> used to do both, it would be something like this:
> 
>     (save-excursion
>         ...
> 	(goto-char x)
>         ...
>         (set-buffer B)
>         (save-excursion
>            ....))
> 
> A second save-excursion would be needed after switching to B, because
> whatever is done afterwards is *not* going to be protected by the
> original save-excursion.

"Whatever is done afterwords" is too abstract.  If you mean point movements in
the original buffer, then of course that is "protected" by the `save-excursion'.
Likewise if you mean buffer changes (which buffer is current) or mark changes
(in the same buffer).

If you mean anything else by "whatever is done afterwards" then no, of course
not.  `save-excursion' has no contract to do anything other than save and
restore which buffer is current as well as its point and its mark.

> > > What makes the buffer A special?  Why is it that you should
> > > always restore the point in the current buffer and leave 
> > > all the other buffers to their fate?
> > 
> > That is what `save-excursion' does.  If you want something
> > different, then code something different.  By hypothesis, the
> > original programmer used `save-excursion', and knew what s?he was
> > doing.  Don't ask me why s?he felt that A needed restoring and not
> > other buffers.  But s?he did (by hypothesis), or s?he wouldn't have
> > used `save-excursion'.
> 
> We shouldn't call `save-excursion' willy-nilly just because it
> exists.

You are full of "should not"s.  But yes, we should not do anything willy-nilly.
And so?  Please, no one is forcing you to call `save-excursion' - even once.

> If the buffer A is indeed special and we should restore only
> its excursions but not those of other buffers, then the right place to
> protect them is wherever the excursions are happening, not at the
> top-level.  

It depends.  And it's the programmer's call.  In general, other things being
equal, yes, limit the scope of a `save-excursion', by all means.  But the devil
is in the details.  We've been through all of this - how many times now?

> Morever, what if that is not what we want?  What if we want to protect
> the excursions in all the buffers, not only the current-buffer of
> `m-b-f'?  The top-level `save-excursion' is not going to help you
> achieve that.

Of course not.  You are repeating what I said.  If you don't want what
`save-excursion' offers then just say no - don't use it.

> What do you do then?

Code it up.  If you need to save and restore several buffers and their points,
then do it.

You will notice that there is no predefined function to do that.  Does that tell
you something?  But you can certainly do it if you need that behavior.

> On a closer look, this kind of programming doesn't look well-thought
> out at all.  It seems quite arbitrary.

Here we go again.

Take a final close look if you like, and decide whatever you like.  I don't
think you're going to get it, no matter how many times we go round.  If you're
happy, great; that's what counts.

`save-excursion' is what it is.  If you don't like it, I certainly won't try to
make you like it.  No one forces you to use it.

I'm happy with `save-excursion', but you need not be.  You can be happy without
it - just don't use it.  I'm happy to use `save-excursion',
`save-current-buffer', `with-current-buffer', and, yes, `set-buffer.

We can both be happy, in our different ways.

> > Let me repeat: `save-excursion' is _not_ `save-current-buffer'.  You
> > substitute the latter for the former willy-nilly at your own peril.
> 
> I know.  I was writing to Stefan earlier today saying exactly that.
> `save-excursion' should be replaced by `save-current-buffer' only
> after ensuring that all excursions are properly protected.  I have
> described my clean-up procedure earlier in the message.
> 
> > Dunno what to say, Uday, without repeating myself more.
> > `save-excursion' saves and restores point in only one buffer.  If
> > that is not the intention then don't use it.  What you call
> > "covering up" is exactly the ignoring of temporary point movements
> > (for _one_ buffer).
> 
> "Only one buffer" is a mistaken assumption.  Typically, in this kind
> of code, there would be save-excursions all over the place.  Whenever
> point movements happen, they would be under a thick stack of
> save-excursions for all kinds of buffers.  The buffer you are doing
> excursions in will typically end up being in that stack accidentaly
> and so its excursion will get cancelled before the whole process ends.
> Somehow magically the code works, even though you don't know how.
> When you tweak the code a little, some buffer might get eliminated
> from the stack and, all of a sudden, its excursions will get exposed
> to the user.  "No problem!" says the hacker.  Slap on another
> save-excursion, and the code starts to work magically again.

Whew!  Feel better?

I don't see anyone coding like that.  But you are welcome to.

> > What if you need to save and later restore point in a 
> > buffer, Uday, what do _you_ do?
> 
> What do _I_ do?  I never need to save and restore the point in a
> buffer.

Ah!  Great.  So you will never need to use `save-excursion'.  Problem solved.
Presumably the same is true for all of that code you maintain - it never needs
to restore point.  Hallelujah!  You're saved!  Rejoice.

> I always place my save-excursions next to the excursions.

Wait a minute.  You just said you never need to save and restore point.  So why
are you still calling `save-excursion'?  I thought you were cured.  Having a
relapse?  Second thoughts?

> Only those point movements that are supposed to be the inteded effects
> of the operations will be outside any save-excursion.

Well, yeah.  That certainly makes sense.  We use `save-excursion' only when we
want to save and restore point (and...).  And typically we use it when we do
want to save and restore point (and...).

> If I make any mistakes in doing this, I go find them and fix them.
> So, I don't need any save-excursions at the top-level.

Oh that's what you meant: top level - you didn't say that.  So you still use
`save-excursion', but just not at the top level.  I have no problem with that.
I don't recall ever using it or even ever seeing it at the top level, but maybe
you mean something special by top level.

> The code that I maintain works exactly in that way.
> And that is what the Elisp manual says you should do as well:
> 
>        It is often useful to move point "temporarily" within a
>        localized portion of the program, or to switch buffers
>        temporarily.  This is called an "excursion", and it is done
>        with the `save-excursion' special form.

(Where's the "top level" here?)

Anyway, I certainly have no problem with that text.  So maybe it's a good place
to end - on common ground.

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

[Stefan Monnier]

> pointing to other ways of delivering a task doesn't prove

> save-excursion+set-buffer

> is wrong.

There is no proof that save-excursion+set-buffer is wrong, because it
just is not wrong: it's just a combination of function that performs
something.  That's why the byte-compiler emits a warning and not an
error message.

What there is, OTOH, is damning evidence that:
- it can be replaced by with-current-buffer in more than 90% of the
  cases (which is more concise and more efficient).
- in the remaining cases it is usually hiding an error and that error
  can be fixed by using (with-current-buffer <foo> (save-excursion ...)).
- in my more than 10 years of maintaining Emacs I have not found
  a single case where the semantics of save-excursion+set-buffer is
  indeed exactly what we're after (i.e. where it's used without
  introducing a bug and without introducing an inefficiency fixable by
  with-current-buffer).

So the warning was introduced to help Elisp coders improve their code.
That's what byte-compiler warnings are for.


        Stefan

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

[Stefan Monnier]

> Given the assumptions that I added (fairly, I hope), I would say that
> presumably the `save-excursion' is there NOT ONLY to restore which
> buffer was current but also restore point and mark in that buffer.
> That's what `save-excursion' DOES, and there is no reason not to
> suppose that the original author did not intend that.

Actually, there is a lot of reason to suppose that the author did *not*
intend it: although the author most likely knew it also did such
a save-point-and-mark, she most likely used save-excursion as
a special-form that provides a super-set of what she needed.  That's the
case for the enormous majority of save-excursion+set-buffer.


        Stefan

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

[David Kastrup]

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

>> pointing to other ways of delivering a task doesn't prove
>
>> save-excursion+set-buffer
>
>> is wrong.
>
> There is no proof that save-excursion+set-buffer is wrong, because it
> just is not wrong: it's just a combination of function that performs
> something.  That's why the byte-compiler emits a warning and not an
> error message.

[...]

> So the warning was introduced to help Elisp coders improve their code.
> That's what byte-compiler warnings are for.

It fails to do so.  It does not _help_ Elisp coders improve their code
but rather _confuses_ them into improving their code.

The warning message would be more accurately written as

     "Warning: the maintainer general has detected that
               save-excursion+set-buffer may seriously hamper your code."

That's much more likely to be accurate than the current warning.

-- 
David Kastrup

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

[PJ Weisberg]

On 3/15/11, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> So the warning was introduced to help Elisp coders improve their code.
> That's what byte-compiler warnings are for.

It's hard to tell what's wrong with the code based on this warning,
though.  (It says save-excursion is "defeated", but buffer, point, and
mark still seem to be getting restored.  So what's the hidden danger?)
 Maybe it should specifially say something like, "save-excursion +
set-buffer could possibly be replaced by with-current-buffer."

-PJ

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

[Drew Adams]

> > Given the assumptions that I added (fairly, I hope), I 
> > would say that presumably the `save-excursion' is there
> > NOT ONLY to restore which buffer was current but also
> > restore point and mark in that buffer.  That's what
> > `save-excursion' DOES, and there is no reason not to
> > suppose that the original author did not intend that.
> 
> Actually, there is a lot of reason to suppose that the author 
> did *not* intend it: although the author most likely knew it
> also did such a save-point-and-mark, she most likely used
> save-excursion as a special-form that provides a super-set of
> what she needed.  That's the case for the enormous majority
> of save-excursion+set-buffer.

Even if that were true for some very old code, there is no reason to bring
`set-buffer' into it.

If someone is NOT interested in coming back to the same point and is interested
only in coming back to the same buffer, then s?he will nowadays naturally use
`save-current-buffer' or `with-current-buffer'.  No problem.

Misplaced authoring such as you describe is generally in old code.  It should be
enough for us to remind users, in the manual, of the existence of
`save-current-buffer' etc.  They will naturally use it to save and restore the
current buffer.  They won't even look at `save-excursion' for that (based on the
name etc.).

If someone uses `save-excursion', nowadays at least, it is purposefully to
return to the same point.  And that's correct usage.  We should not assume that
they are mistakenly using it only to restore the buffer, even if such an
assumption might be a reasonable test-guideline for someone like yourself who is
trying to clean up old code.

And even for such a search-and-destroy old code mission, looking for
`set-buffer' is by no means sufficient, as I'm sure you realize.  You might as
well just look for `save-excursion' and check to see if each occurrence really
needs to save point.

`set-buffer' is a red herring here - totally.  It is neither necessary nor
sufficient as an indicator of any problem.  It does not matter what happens
during a `save-excursion' excursion - it really doesn't.  `set-buffer',
`jump-off-the-deep-end', `go-fly-a-kite', or whatever.  It just does not matter.

`save-excursion' brings you back where you were, and that's all it does.  Don't
expect more, or less, from it.  If that's the behavior you want, then
`save-excursion' is called for.  If you do not want or need to come back, then
don't use `save-excursion'.

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

[Drew Adams]

> There is no proof that save-excursion+set-buffer is wrong, because it
> just is not wrong: it's just a combination of function that performs
> something.

Correct.

> That's why the byte-compiler emits a warning and not an
> error message.

A warning about what?  There is not only no proof that save-excursion+set-buffer
is wrong - it is not at all wrong.  As you admit.

> What there is, OTOH, is damning evidence that:
> - it can be replaced by with-current-buffer in more than 90% of the
>   cases (which is more concise and more efficient).

Cases where?  In old Emacs source code?

Really, you don't have to tell people to replace `save-excursion+set-buffer'
with `with-current-buffer', especially since such a replacement might not be
appropriate.  

Just tell people of the existence of `with-current-buffer' and
`save-current-buffer', and they will naturally use those for their intended
purpose.  People will not naturally use `save-excursion' nowadays just to save
the current buffer.

> - in the remaining cases it is usually hiding an error and that error
>   can be fixed by using (with-current-buffer <foo> 
>  (save-excursion ...)).

In the remaining cases in old Emacs source code?  See above.

> - in my more than 10 years of maintaining Emacs I have not found
>   a single case where the semantics of save-excursion+set-buffer is
>   indeed exactly what we're after (i.e. where it's used without
>   introducing a bug and without introducing an inefficiency fixable by
>   with-current-buffer).

You are fixated on `set-buffer'.  `save-excursion' doesn't care what you do
while excursioning.  You can `set-buffer' all you want or not change buffers at
all.  It simply doesn't matter.  All that's important about `save-excursion' is
what it does: bring you back.

> So the warning was introduced to help Elisp coders improve their code.
> That's what byte-compiler warnings are for.

It won't help, but will hurt, at least with users writing new code.  If someone
uses `save-excursion' nowadays it is only in order to do what `save-excursion'
does: return you where you were.  There is nothing wrong with calling
`set-buffer' (or anything else) inside a `save-excursion'.

Your real message should be just to remind people that `save-excursion' does not
do more than it does - e.g., remove any misconceptions that it saves and
restores positions in other buffers.

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

[Stefan Monnier]

>> So the warning was introduced to help Elisp coders improve their code.
>> That's what byte-compiler warnings are for.

> It's hard to tell what's wrong with the code based on this warning,
> though.  (It says save-excursion is "defeated", but buffer, point, and
> mark still seem to be getting restored.  So what's the hidden danger?)
>  Maybe it should specifially say something like, "save-excursion +
> set-buffer could possibly be replaced by with-current-buffer."

Indeed, I've changed it yesterday (or so) in Emacs trunk to drop the
"defeat" verbiage and just recommend to replace it with
with-current-buffer, based on Uday's earlier suggestion.


        Stefan

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

[Stefan Monnier]

>> Actually, there is a lot of reason to suppose that the author 
>> did *not* intend it: although the author most likely knew it
>> also did such a save-point-and-mark, she most likely used
>> save-excursion as a special-form that provides a super-set of
>> what she needed.  That's the case for the enormous majority
>> of save-excursion+set-buffer.

> Even if that were true for some very old code, there is no reason to bring
> `set-buffer' into it.

Of course there is: what I wrote is only true when set-buffer is used.
When it's not used in the save-excursion, then it's actually very common
for that save-excursion to indeed be used to save point (I have the
strong feeling that saving the mark is  complete waste for 99% of
save-excursions, but I haven't yet attacked this so I don't actually
know for sure).


        Stefan

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

[Stefan Monnier]

> People will not naturally use `save-excursion' nowadays just to save
> the current buffer.

Maybe not naturally, no.  But thanks to the pressure of years of old
code flying around the web, they will do it nowadays on
a regular basis.  But indeed, thanks to threads like this one and to the
warning I added, I hope I can reverse the trend.


        Stefan

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

[Drew Adams]

> > Even if that were true for some very old code, there is no 
> > reason to bring `set-buffer' into it.
> 
> Of course there is: what I wrote is only true when set-buffer is used.
> When it's not used in the save-excursion, then it's actually very
> common for that save-excursion to indeed be used to save point

"when it's not used in the save-excursion" is not the same thing as "when I
don't see it in the `save-excursion' sexp.

As I said:

> > And even for such a search-and-destroy old code mission,
> > looking for `set-buffer' is by no means sufficient, as I'm sure
> > you realize.  You might as well just look for `save-excursion'
> > and check to see if each occurrence really needs to save point.
> > 
> > `set-buffer' is a red herring here - totally.  It is neither
> > necessary nor sufficient as an indicator of any problem.  It
> > does not matter what happens during a `save-excursion' excursion
> > - it really doesn't.  `set-buffer', `jump-off-the-deep-end',
> > `go-fly-a-kite', or whatever.  It just does not matter.

Even if you are worried about `set-buffer' (which you need not be), you won't
necessarily find it just by looking for it literally and lexically in the
`save-excursion' sexp.  But you know this.

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

[Drew Adams]

> > People will not naturally use `save-excursion' nowadays just to save
> > the current buffer.
> 
> Maybe not naturally, no.  But thanks to the pressure of years of old
> code flying around the web, they will do it nowadays on
> a regular basis.  But indeed, thanks to threads like this one 
> and to the warning I added, I hope I can reverse the trend.

The warning has already mistaught some (e.g. Uday) that `save-excursion' is just
downright evil.  The warning leaves users either unnecessarily frightened or
unnecessarily confused, or both.  That was not your intention, I'm sure, but it
is the effect.

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

[Jason Earl]

On Tue, Mar 15 2011, Stefan Monnier wrote:

>> People will not naturally use `save-excursion' nowadays just to save
>> the current buffer.
>
> Maybe not naturally, no.  But thanks to the pressure of years of old
> code flying around the web, they will do it nowadays on a regular
> basis.  But indeed, thanks to threads like this one and to the warning
> I added, I hope I can reverse the trend.
>
>
>         Stefan

Precisely, these warnings are not for people like you, Drew, or David.
You guys have the experience necessary to make the correct choice.
Warnings like this are *very* helpful to people like me, however, that
have no experience with Emacs Lisp, but would like to write Elisp
anyhow.

To be honest, for the purpose of teaching newbies it almost doesn't
matter what the warning says either.  As long as it is clear that
save-excursion and set-buffer are not to be used together then it is a
win.

And yes, I realize that what I should do is read the manual umpteen
million times, and memorize a pile of stuff.  Actually, in this case
this might not be such a good idea.  I assumed that I had picked up the
save-excursion + set-buffer idiom while I was perusing the Gnus code,
but in checking the manual I think that I might have picked it up from
the /Introduction to Emacs Lisp/.  Using save-excursion and set-buffer
together is covered in:

         4.4.3 `save-excursion' in `append-to-buffer'

Jason

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

[Drew Adams]

> >> People will not naturally use `save-excursion' nowadays 
> >> just to save the current buffer.
> >
> > Maybe not naturally, no.  But thanks to the pressure of years of old
> > code flying around the web, they will do it nowadays on a regular
> > basis.  But indeed, thanks to threads like this one and to 
> > the warning I added, I hope I can reverse the trend.
> 
> Precisely, these warnings are not for people like you, Drew, or David.
> You guys have the experience necessary to make the correct choice.
> Warnings like this are *very* helpful to people like me, however, that
> have no experience with Emacs Lisp, but would like to write Elisp
> anyhow.

Actually, it is precisely people such as yourself who are done the greatest
disservice by this warning, IMHO.

> To be honest, for the purpose of teaching newbies it almost doesn't
> matter what the warning says either.  As long as it is clear that
> save-excursion and set-buffer are not to be used together then it is a
> win.

Et voila the damage done.  It is NOT true that "`save-excursion' and
`set-buffer' are not to be used together."  Even Stefan acknowledges that.  But
that's not the message that you (_clearly_) received.

> And yes, I realize that what I should do is read the manual umpteen
> million times, and memorize a pile of stuff.  Actually, in this case
> this might not be such a good idea.

Just read the doc string of `save-excursion' and you should be good to go.  It
tells you all you will ever need to know about it.  It tells you what
`save-excursion' _does_.  

The warning was supposedly designed to hint at what `save-excursion' does _not_
do, e.g., to prevent you from having unreasonable expectations that it will
restore more than it does.

> I assumed that I had picked up the save-excursion + set-buffer idiom
> while I was perusing the Gnus code, but in checking the manual I
> think that I might have picked it up from the /Introduction to
> Emacs Lisp/.  Using save-excursion and set-buffer together is covered in:
> 
>          4.4.3 `save-excursion' in `append-to-buffer'

Yes, please file a (doc) bug: the example in that section does not reflect the
latest definition of `append-to-buffer', which now uses `with-current-buffer'
instead of `set-buffer'.

That said, the code shown in that doc, which is just the Emacs 22 version of
`append-to-buffer', is not bad code.  It uses `set-buffer' instead of
`with-current-buffer', AND there is no harm in it (IMHO):

Emacs 22:
(save-excursion
  (set-buffer append-to) ; <======
  (setq point (point))
  (barf-if-buffer-read-only)
  (insert-buffer-substring oldbuf start end)
  (dolist (window windows)
    (when (= (window-point window) point)
      (set-window-point window (point))))))

Emacs 23+:
(save-excursion
  (with-current-buffer append-to ; <======
    (setq point (point))
    (barf-if-buffer-read-only)
    (insert-buffer-substring oldbuf start end)
    (dolist (window windows)
      (when (= (window-point window) point)
        (set-window-point window (point))))))

Do I prefer the latter (Emacs 23+)?  Yes.
Is the former dangerous or abominable?  No.

Does it even matter whether the BUFFER arg to `append-to-buffer' (which
corresponds to APPEND-TO above) happens to be the same as the `current-buffer'?
No.

Did this code _really need_ to be "fixed" to use `with-current-buffer' instead
of `set-buffer'?  No.  No danger, but it's still better with
`with-current-buffer'.

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

[Stefan Monnier]

> The warning has already mistaught some (e.g. Uday) that
> `save-excursion' is just downright evil.

Don't worry: Uday is a big boy.  He understands the issue at least as
well as you do, probably much better.


        Stefan

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

[rusi]

On Mar 16, 12:02 am, Jason Earl <je...@notengoamigos.org> wrote:
> On Tue, Mar 15 2011, Stefan Monnier wrote:
> >> People will not naturally use `save-excursion' nowadays just to save
> >> the current buffer.
>
> > Maybe not naturally, no.  But thanks to the pressure of years of old
> > code flying around the web, they will do it nowadays on a regular
> > basis.  But indeed, thanks to threads like this one and to the warning
> > I added, I hope I can reverse the trend.
>
> >         Stefan
>
> Precisely, these warnings are not for people like you, Drew, or David.
> You guys have the experience necessary to make the correct choice.
> Warnings like this are *very* helpful to people like me, however, that
> have no experience with Emacs Lisp, but would like to write Elisp
> anyhow.
>
> To be honest, for the purpose of teaching newbies it almost doesn't
> matter what the warning says either.  As long as it is clear that
> save-excursion and set-buffer are not to be used together then it is a
> win.
>
> And yes, I realize that what I should do is read the manual umpteen
> million times, and memorize a pile of stuff.  Actually, in this case
> this might not be such a good idea.  I assumed that I had picked up the
> save-excursion + set-buffer idiom while I was perusing the Gnus code,
> but in checking the manual I think that I might have picked it up from
> the /Introduction to Emacs Lisp/.  Using save-excursion and set-buffer
> together is covered in:
>
>          4.4.3 `save-excursion' in `append-to-buffer'

Aha so that's the culprit
 (info "(eintr)append save-excursion")

A 30 year old project has 30 years of tradition, 30 years of of
stability...
And 30 years of cruft.
Anyone for a heavy-duty coarse-grade scrub?

Thanks Jason

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

[David Kastrup]

rusi <rustompmody@gmail.com> writes:

> Aha so that's the culprit
>  (info "(eintr)append save-excursion")
>
> A 30 year old project has 30 years of tradition, 30 years of of
> stability...
> And 30 years of cruft.
> Anyone for a heavy-duty coarse-grade scrub?

Who is going to translate all that Elisp into Lua?

-- 
David Kastrup

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

[Uday S Reddy]

Drew Adams writes:

> The warning has already mistaught some (e.g. Uday) that
> `save-excursion' is just downright evil.  The warning leaves users
> either unnecessarily frightened or unnecessarily confused, or both.
> That was not your intention, I'm sure, but it is the effect.

Hi Drew, that is a complete misunderstanding, if not a downright
distortion, of what I wrote.  `save-excursion' to protect point
movements and buffer changes that make up an "excursion" are perfectly
fine.  That is indeed the intended application of `save-excursion' as
described in the ELisp manual.  I have even shown sample pieces of
code describing how and where `save-excursion' should be placed.  For
example: 

Uday S Reddy writes:

> No, it wasn't from an actual application.  I just made it up.  And, it
> isn't good code either.  To make it good code, one needs another
> save-excursion to protect the point movement in buffer A, as below:
> 
>      (defun find-something ()
>         (....
>          (save-excursion 
>               (set-buffer A)  
>               (save-excursion
>                  (goto-char y)  
>                   ...))))
> 

You seem to have missed the significance of these statements.

Cheers,
Uday

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

[rusi]

On Mar 16, 1:11 pm, David Kastrup <d...@gnu.org> wrote:
> rusi <rustompm...@gmail.com> writes:
> > Aha so that's the culprit
> >  (info "(eintr)append save-excursion")
>
> > A 30 year old project has 30 years of tradition, 30 years of of
> > stability...
> > And 30 years of cruft.
> > Anyone for a heavy-duty coarse-grade scrub?
>
> Who is going to translate all that Elisp into Lua?
>
> --
> David Kastrup

Lua? Whoa!   Thats a super-heavy-duty scrub :-)
Anyhow if thats the chosen route, a path could be with an interim dual-
language period -- as follows:

1. Add lua sources into emacs -- relatively trivial
2. Build data-structure bridges for communicating data structures
between elisp and lua
3. Code up something like python's 2to3 http://docs.python.org/library/2to3.html
for translating most of elisp to lua
[Note: most is doable, all is not]

Such bridges already exist -- eg pymacs, eclim and emacs-eclim.

Anyhow -- dunno if this -- elisp-obsolescence -- is the main/real
cruft...

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

[rusi]

On Mar 17, 8:46 am, rusi <rustompm...@gmail.com> wrote:
> Lua?
> <snipped>
> Anyhow -- dunno if this -- elisp-obsolescence -- is the main/real cruft...

In particular, this thread's topic itself -- save-excursion, set-
buffer -- does it have anything specifically 'lispy' about it?  How
would a non-lisp language obviate this kind of problem except for the
advantages of starting over?

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

[Antoine Levitt]

17/03/11 08:10, rusi
> On Mar 17, 8:46 am, rusi <rustompm...@gmail.com> wrote:
>> Lua?
>> <snipped>
>> Anyhow -- dunno if this -- elisp-obsolescence -- is the main/real cruft...
>
> In particular, this thread's topic itself -- save-excursion, set-
> buffer -- does it have anything specifically 'lispy' about it?  How
> would a non-lisp language obviate this kind of problem except for the
> advantages of starting over?

He was just kidding. :)

(FWIW, from my limited experience, the "advantages" of starting over are
extremely relative.)

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

[rusi]

There is this 'new' save-excursion issue/discussion/bug here:
http://thread.gmane.org/gmane.emacs.orgmode/40417

I wonder how much it is related to this discussion?

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

[Uday Reddy]

On 4/1/2011 4:20 AM, rusi wrote:
> There is this 'new' save-excursion issue/discussion/bug here:
> http://thread.gmane.org/gmane.emacs.orgmode/40417
>
> I wonder how much it is related to this discussion?

It is not exactly related.  It is a probably something similar to this:

(save-excursion
   (let ((x (buffer-substring (point-min) (point-max))))
     (erase-buffer)
     (insert x)))

What does it do?  Well, save-excursion saves the point, but then 
erase-buffer destroys the point.  So, the "point" moves to the beginning 
of buffer.  And, save-excursion makes sure it stays there.

Without save-excursion, the point would move to the end of the buffer 
because of (insert x).

In the org-mode example in the thread, org-agenda-redo is probably doing 
a good job of putting the point at a good place.  But the save-excursion 
at the top-level is negating all its good work.

It just goes to show the fallacy of thinking that save-excursion doesn't 
hurt.

Cheers,
Uday