bug#25777: 25.1; [PATCH] `rectangle--pos-cols' should not move point

[Drew Adams <drew.adams@oracle.com>]

> > But IMHO, it is generally better to scope a `save-excursion'
> > as tightly as possible around the movement that you want to
> > control (hide, erase, undo).
> 
> Well, my opinion is just about the opposite, but I won't block
> the patch for that.
> 
> > Why?  Because it makes the code much clearer.  It tells you
> > that outside the `save-excursion' zones point is unlikely
> > to be moved.  And that makes maintenance easier and less
> > error-prone.
> 
> I find wider scoping of `save-excursion' to be clearer.  It tells you
> that the code outside the save-excursion zone cares about the original
> value of point.

It cannot really tell you that code outside the zone cares about
point.  That's not what `s-e' does.  `s-e' only acts on code
inside it.  It can only tell you that code inside it will not
move point (because of the `s-e').  Which means that any code
outside it CAN care about (rely on) point.

And that applies to the code outside the more narrowly defined
zones too: they CAN care about point.  A maintainer can use the
value of point anywhere outside those narrowly defined zones.

Whether the current code actually cares about point outside
a given `s-e' is less important for maintenence than knowing
where code in general _could_ care about point.

And if do want `s-e' placement to suggest something about
some code outside it actually using point then `s-e', i.e.,
if you want to adopt that as a coding convention (since it
is not something that `s-e' tells you, itself), then such
a convention does a poor job of telling you which outside
code does the using.

The `s-e' would have to be placed so that the _use_ of point
were immediately next to the `s-e' occurrence (_just_ outside
it).  Otherwise, your principle would not be followed - you
would not be masking as much point movement as possible, just
up to where point is actually used.

Such placement is too coarse - far coarser than placement
with the intention (and the actual effect, because it is what
`s-e' does) of indicating where `s-e' NEEDS to protect from
point movement.

But I accord that people can disagree about whether `s-e'
placement should be done only where it is _needed_ or as
widely as possible, just barely beyond where point is used.

> If there are multiple `save-excursion' zones, I have
> to look at what comes after each of them, to see what
> they're using point for.

No, you don't.  What you can be pretty sure of is that
_any_ code there _could_ use point.  IOW, the normal state
of affairs pertains outside the `s-e': you can use point.

The default in Emacs is to not use `save-excursion'
gratuitously.  Code uses it only when/where it matters -
where it's needed.  Limiting its scope to where it really
matters makes it clearer just where/when that is.  There
is no need to wrap wider, so why do it?

> Therefore maintenance becomes harder and more error-prone
> with many smaller save-excursion calls.

That doesn't follow.  Outside those zones, including
anywhere else within the function body, maintenance is free
to use or change point anyway that is called for.

And on the contrary: If you put lots of stuff inside `s-e',
stuff that need not use point, then when you go to make a
change to something there you have to figure out whether -
at that position inside - you could use point if you moved
the `s-e' a bit etc.  Where `s-e' is should tell you, "Here
we're moving point only temporarily.  We undo any such
movement, so that beyond this zone you can continue to use
it normally."

> With a single save-excursion, it's immediately clear that
> the value of point is unchanged by the function.

By the function!  Sure.  But it tells you nothing about
the code within the function.  You seem to be assuming
that all that matters is whether the function moves point.
And that abstracts totally from the possibility of making
future changes to the function code.

What's more: that argument you just made is the same one
I'm making, but for the code where the movement can be
undone.  IOW, you are making the same argument as I, but
you are applying it only to the whole function.  The whole
function _need not_ have point-movement-must-be-undone
imposed on it.  That stops you from making changes to the
code of the function that might need, here or there, to
make use of point.

> And it's clear that adding more code to the function will
> preserve that property, so maintenance is easier.

If you wrap the entire world with a `s-e' then clearly
you can add code to the world and it will still be true
that the world will not move point.

However, you will be unable to treat any of the code in the
function itself as places where you can make _use_ of point.
You will not be able to use point anywhere in the world, if
you've wrapped it all in `s-e'.

> It comes down to doing something
> (grabbing/releasing a mutex or saving/restoring point)
> only where/when it's needed

That's _my_ argument.  I put it only where it is _needed_,
which is where point movement _needs_ to be undone.  You
gratuitously want to envelope also other code, where point
movement does _not_ need to be undone.  Doing that, you tie
your hands for future changes to other parts of the same
function where you might want to use point.

> (which would be once at the beginning and end of the
> function in this case) rather than many redundant times
> (which would be each time goto-char is called).

We seem to have different meanings of "needing" to cancel
(undo) point movement.  It seems clear to me that if only
the parts that actually move point have that movement
canceled the result is correct behavior.  Which means that
there is no need to wrap more code.  Wrapping more code is
gratuitous, not necessary.

And those places where point is moved can be independent.
There is nothing inherently "redundant" about them.

> > If you confirm that you really want that wider scope here
> > for `s-e' then I'll do that.  Otherwise, I'll keep the
> > `s-e' occurrences where they are but do the renaming and
> > add a doc string.  Let me know.  Thx.
> 
> As I've explained, I prefer wider scope.  However, the Emacs project
> doesn't have a policy on this as far as I know, so we're in a similar
> situation with pcase vs cond thing: the person making the patch makes
> the final decision.  If you found my arguments for it unconvincing,
> and still feel strongly that narrower scope is better, I'll accept a
> patch with the small scopes too.

And I feel just as accommodating. ;-)  I wanted to make my
argument, and have done so, but I really don't care what is
done here to fix the bug.

My suggestion, since I think we've both had our say and are
unlikely to convince one another on the level of principle
or guideline, is for you to please make the fix.

Since it is you who will kindly make the actual code change
and commit, you should/can do it however you like.  I have
no problem with that.

But do you really need another patch for that, especially
since wrapping widely, a single time, is so much simpler? ;-)
Could you please make the change?  Your way is fine by me,
for this.  Thx.