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

[Tim Cross <theophilusx@gmail.com>]

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

On Mon, Mar 14, 2011 at 11:26 AM, Drew Adams <drew.adams@oracle.com> 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

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