emacs-devel
[Top][All Lists]
Advanced

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

Re: save-excursion and save-current-buffer - edits to the manual


From: Tim Cross
Subject: Re: save-excursion and save-current-buffer - edits to the manual
Date: Mon, 14 Mar 2011 12:17:04 +1100



On Mon, Mar 14, 2011 at 11:26 AM, Drew Adams <address@hidden> wrote:
> The point is merely to make it faithful to the compiler...
> ... it contradicts the compiler warning by saying explicitly that
> one can use save-excursion whereas the compiler says no.
>
> > My comment is to please leave it as it was.
>
> We can't, because it contradicts the compiler.
>
> > Both [functions] can be useful here.
>
> No, the compiler complains about save-excursion being used
> for this purpose.

So now we've apparently gone all the way - from a warning issued supposedly just
in case, because a tiny minority of cases where `save-excursion' is used with
`set-buffer' are deemed (by some) to be inappropriate or not ideal, to your
claiming outright that use of `save-excursion' "contradicts the compiler" and
"the compiler says no", it simply cannot be used.

This is exactly how we go (quickly, it seems) down the road from carefully
reasoned judgment calls to black & white catechism.  Sheesh.  Those who come
later will have no inkling of understanding (certainly not from your addition to
the manual), but will no doubt feel proud to parrot "The Rule".  Sorry, but this
is a disservice.

You yourself have indicated that of the zillions of cases the compiler flagged
for you in the code you inherited, you changed only a small minority in the end.
So certainly it is not a no-no to use `save-excursion' with `set-buffer'.  Even
by your own estimation of what was needed in your code.

Sorry, but both `save-excursion' and `save-current-buffer' _can_ be useful here.
And it is simply a bad idea to purge the manual this way.  Regardless of what
the compiler has been made to say or suggest recently.  The manual will
hopefully offer more understanding about this than the compiler does!

And the manual does so already, in the doc of `save-excursion'.  It is very
clear, from both the manual and the doc string, what `save-excursion' does.  But
one has to take the trouble to read it.

What is in the body of `save-excursion' is irrelevant to what `save-excursion'
does.  The function simply makes the original buffer, point, and mark current
again when it ends.  To understand that is to understand everything about it,
including how and when to use it.

> > "However, it is not a recommended use of `save-excursion'
> > for reverting buffer changes."
> >
> > The word "it" references nothing here - no meaning.
>
> If you really want "it" to refer to something, think of "it" as the
> compiler.

What do _you_ really want?  "However, _the compiler_ is not a recommended use of
`save-excursion' for reverting buffer changes"?  From awful to worse...

> > And `save-excursion' is not about reverting buffer changes
> > - never has been.  And no one would ever think that it is.
> > And the warning you then try to explain also has nothing to do
> > with reverting buffer changes.
>
> But this section is about buffer changes.  And, in any case, the
> sentence before the one you quoted says:
>
>    "save-excursion restores the current buffer and, in addition,
> restores the point and mark in that buffer as well."

Yes, but none of those things have anything to do with changing the content of a
buffer, which is what _reverting buffer changes_ refers to.  Changing which
buffer is current is not related to reverting buffer changes.  Nor is changing
point or mark.  It is only your proposed addition that introduces "reverting
buffer changes".

> > And your explanation of the warning, like the warning
> > itself, does not help.
>
> Since you think the warning doesn't help,

Believe me, I'm not the only one who thinks that.  Not that it matters...

> I am not surprised that you don't find the explanation helpful.
> But, perhaps, you can be a bit more objective here?  In what way
> does it not help?

See above.  You give the distinct impression that it is _wrong_ to use
`save-excursion' with `set-buffer'.  It is not wrong.  That mistaken impression
does not help.

> > "there should be no calls to `set-buffer' in the intervening code
> >  inside the `save-excursion' form before the changes to
> >  point or mark, because `save-excursion' only restores the point
> >  and mark of the current buffer, not for the other buffers that
> >  may be the target of `set-buffer'"
> >
> > Doesn't follow at all.
>
> It does follow.  Here is the logic, spelled out:

No, here is _your_ logic spelled out - your logic and your words:

1. "`save-excursion' only restores the point and mark of the
  current buffer, not for other buffers that may be the target
  of `set-buffer'."

2. THEREFORE, do not call `set-buffer' inside `save-excursion'
  and then move point or mark.

Look at the quotation just above this to see if I'm not right.
That's just what you said: #1 implies #2; #2 is "because" of #1.

#2 simply does _not_ follow from #1.  No way, no how.

#1 just says what `save-excursion' does, nothing more.  No objection.
#2 is your rule, parachuted in from nowhere.  Nothing in #1 implies #2.

> 1. save-excursion should be placed as close as possible to the
>    point/mark-movements.
> 2. save-excursion only restores the current buffer's point/mark.
>
> So, if there is a set-buffer between the save-excursion and the
> point/mark-movements, then it is not as close as possible to the
> point/mark-movements.

Huh?  First, you are conflating things, bringing in a general principle of
software engineering that no one disagrees with and that does not apply here
specially: do not needlessly extend scope.

More importantly, changing point and mark after invoking `set-buffer' changes
them in another buffer, the target of `set-buffer'.  `save-excursion' has _no
effect_ whatsoever on point and mark in that buffer (unless it is also the
orginal buffer).  Surely that should be clear by now.

It is a good idea (but not cause for any warning) to wrap a construct such as
`save-excursion' as tightly as possible around the things it is intended to
undo/restore.  No one argues the contrary.

If the `set-buffer' is to be reversed, i.e., the original buffer is to be
restored after the `set-buffer', then the scope must extend beyond the
`set-buffer', but certainly not because of any point or mark movements in that
buffer - `save-excursion' has no effect on them.  (Unless of course it is the
same buffer.)

And if there is code after the `set-buffer' that is also in the category of
changes to be reversed/restored, then the scope should be closed only after that
as well.  If not, then its scope should be ended before that.  But again,
`save-excursion' cannot restore anything more than (a) which buffer was
originally current, (b) its point, and (c) its mark.

At any rate, none of this is what the warning is about, according to the various
defenses of it that have been presented.  The warning does not say "The scope of
`save-excursion' is looser than it needs to be in this occurrence".  If it did,
and it were accurate, I would likely be in favor of it (but still not as a
_warning_).

> (In fact, in the normal cases where the
> set-buffer's are not no-op's , there would have to be at least *two*
> set-buffer's between the save-excursion and the point/mark-movements
> that it is reverting.)

Huh?  I have no idea what you are talking about now.  `save-excursion' does not
revert point/mark in any buffers except the original one.  How many
`set-buffer's there are and which buffers become current at different points in
the body are irrelevant.  And that includes any `set-buffer's back to the
original buffer.  The only point movements in the body of a `save-excursion'
that are affected (rolled back) are those in the orginal buffer.

`save-excursion' always makes the original buffer current again and always
restores its original point and mark.  Only.  Nothing more.

What you do in the body of the `save-excursion' has no effect on this
restoration action (well, you could kill the original buffer...), and
`save-excursion' has no other action.  None.

> > This is getting sillier and sillier.  All because of a
> > warning that no one can decipher and the explanations for
> > which go round and round...nowhere.
>
> Sorry, I have been able to decipher it just fine.

Yeah, and there are those who claim to hear directly from Ms Supreme Being and
impart Her wisdom to us mortels who cannot hear Her.  But each such prophet
hears a different story - go figure.  Kool Aid.

I'm sure you're deciphering.  I'm not sure you're deciphering the same thing as
others (there have been several different explanations/interpretations of what
is being warned about, including a few by Stefan himself).

But even if you are deciphering exactly what Stefan intends that doesn't make
the warning a good one.  It doesn't even make the idea of trying to have such a
warning a good idea.  The warning idea is entirely misguided, IMHO.

What's called for is making clear what `save-excursion' does and does not do.
That's all.  Help users understand what really happens and it will be clear
enough to them how and when to use it.  Will there be zero mistakes made then?
No.  But there is nothing to warn about, nothing at all.

> On the other hand, I haven't been able to decipher what your
> objection is about.

And yet I've been explicit.  Let me repeat it for you:

1. I object to having the warning at all (annoying, misleading).

2. I object to the warning's being a WARNING rather than an informative msg.
There is no DANGER here.

3. I object to the warning's not doing any good: it is unable to target real or
even possible problems narrowly.  Catching all occurrences of `save-excursion'
with `set-buffer' indicts far more innocent than guilty cases, even by Stefan's
(and your own previous) judgment.  It is a blanket alarm.

4. I object to the warning's doing real harm:

(a) It and you seem to claim that using `save-excursion' with `set-buffer' is
always or even usually a bad idea.  And that's just not true.

(b) It scares users when they byte-compile a library, and scares them only
abstractly, without imparting any understanding of what the supposed dangers are
- it's just a big, wild, noisy, scary scream: DANGER!!! LOCK THE DOOR!

(c) Overuse of WARNINGs for things that are not dangerous dilutes the meaning of
warnings - ask Chicken Little and the Boy Who Cried "Wolf!".

What would you think of a loud speaker and flashing red lights in your car
announcing every 30 seconds while you drive, "ATTENTION!! DRIVING CAN BE
DANGEROUS"?  Think it's helpful?  And yet in that case there is a _real danger_!

5. I object to your interepreting the problem, in the manual, as being about
placing `save-excursion' closer to point movements (regardless of which buffer
they are in).  That is not the crux of the problems cited by Stefan or others.
That's just sane coding practice - it applies equally to `let', and it certainly
has nothing to do with a warning or danger.  And the warning does not reflect
scope tightness; it is a knee-jerk alarm.


It would seem two different issues are being mixed up here. 

Issue 1 appears to be a debate concerning whether the situation covered by the warning is serious enough or sufficient to be classified as a warning.  At best, I suspect it is borderline - possibly even less so given that it appears to be somewhat 'simplistic' and cannot identify situations where set-buffer within a save-excursion is legit from those where it is either the source of an error or could be replaced with just a with-current-buffer.  I do think the information can be useful and would have no issue with it being reported as a notice or even removed and only included in something like elint.  The distinction between notice and warning is blurred and changing to notice may not actually help - I tend to think of a notice as more like information rather than a warning of a possible problem. Whether it is a warning, a notice or removed altogether, is largely irrelevant from my standpoint.

Issue 2. is, given that the compiler does report this warning, is the manual clear enough or detailed enough that the programmer can understand what the warning means and when to be concerned or not. I think this is the real issue that needs to be addressed. I've not read Uday's suggested contribution and therefore won't comment on it. However, as things currently stand, I don't think the manual addresses the matter sufficiently and that without this, the warning will continue to be a source of confusion (and probably future debate). Either the warning should be removed (which seems unlikely) or we try to enhance the manual to explain what the warning means and provide the background information that would prevent confusion and possibly make the warning more useful. 

Lets separate the two issues and deal with them individually. Those who want to debate the merits/validity of the warning can do so. Those who want the warning to be less confusing and possibly more informative might like to try and suggest patches to the manual. 

Tim



reply via email to

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