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

[npostavs]

On Tue, Feb 28, 2017 at 10:11 AM, Drew Adams <drew.adams@oracle.com> wrote:
>
> 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.  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.  Therefore maintenance becomes harder and more error-prone
with many smaller save-excursion calls.

With a single save-excursion, it's immediately clear that the value of
point is unchanged by the function.  And it's clear that adding more
code to the function will preserve that property, so maintenance is
easier.

> Putting a `s-e' at a wider location is analogous to putting
> a mutex block at an unnecessarily wide location.  (Yes,
> there is no real connection between those two, but it comes
> down to doing something only where/when it's needed.)

I agree they are roughly analogous, but again come to the opposite
conclusion.  It would be simpler and clearer to make the mutex as wide
as possible without causing deadlock, but for performance reasons, we
choose as narrow a scope as possible.  The performance reasons don't
apply to save-excursion though.  It comes down to doing something
(grabbing/releasing a mutex or saving/restoring point) only where/when
it's needed (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).

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