help-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Drew Adams
Subject: RE: `save-excursion' defeated by `set-buffer'
Date: Mon, 14 Mar 2011 20:55:59 -0700

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]