electric-indent-post-self-insert-function: a partial code review.

electric-indent-post-self-insert-function: a partial code review.

[Alan Mackenzie]

Hi, Emacs.

The doc string of `electric-indent-inhibit' says:

    "If non-nil, reindentation is not appropriate for this buffer."

.  This is vague and wishy-washy.  What, exactly, does "not appropriate"
mean?  When non-nil, does reindentation get done, or doesn't it?  Better
would be:

    "If non-nil, electric reindentation is not done in this buffer."

, if this is in fact what is intended.

#########################################################################

The doc string of `electric-indent-functions-without-reindent' says:

    "List of indent functions that can't reindent."

.  Even though the rest of the doc string explains what is meant, this
top line is nonsensical - all the functions listed _can_ reindent.
Better, I think, would be:

    "List of indent functions which won't be used for reindentation."

, even if not all that much better.  But what is meant by
"REindentation", as opposed to "indentation"?

Also, in `electric-indent-post-self-insert-function', there are two
calls to `indent-according-to-mode'.  `e-i-f-without-reindent' is only
checked for one of these calls.  Is this a bug?

########################################################################

In `electric--after-char-pos', there is the strange looking form:

    (eq (char-before) last-command-event) ;; Sanity check.

.  What is this supposed to check?  After inserting a newline,
(char-before) is 10.  `last-command-event' is (on my Linux tty) either
10 or 13 (after typing C-j or <ret>).  Distingushing them here doesn't
seem to make sense.  What is this form intended to distinguish?

#########################################################################

Assuming the above meaning for `electric-indent-inhibit', then there is
the following problem: even with `e-i-inhibit' set to t, electric
indentation gets done on the new line after insertion of a \n.

To see this, note that `pos' is bound to (electric--after-char-pos),
that is, the position after the last non-ws character inserted into the
buffer.  The last clause inside the outermost `when' is:

      (unless (and electric-indent-inhibit
                   (> pos (line-beginning-position)))
        (indent-according-to-mode)))

.  This will invoke `indent-according-to-mode' when (<= pos
(line-beginning-position)), i.e. when a \n has just been inserted,
regardless of `electric-indent-inhibit'.  This is surely a bug.

#########################################################################

In general, `electric-mode-post-self-insert-function' seems horrifically
and needlessly over-complicated, even though it is only 50 lines long.
The various checks performed before invoking `indent-according-to-mode'
are done at many different places at several different levels of nesting
in the code.  For instance, why is `electric-indent-inhibit' checked
twice at a lower level, rather than just once at the top level?

Why can the various checks not simply be successive arms of an `and'
form?

The reindentation of the original line sometimes happens in the first
call of `indent-according-to-mode', sometimes in the second call.
Perhaps it would be clearer if the original line was always reindented
in the first call, and the new line (if any) in the second call.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: electric-indent-post-self-insert-function: a partial code review.

[Stefan Monnier]

>     "If non-nil, reindentation is not appropriate for this buffer."
> .  This is vague and wishy-washy.  What, exactly, does "not appropriate"
> mean?  When non-nil, does reindentation get done, or doesn't it?  Better
> would be:
>     "If non-nil, electric reindentation is not done in this buffer."
> , if this is in fact what is intended.

Feel free to improve it, yes.

> #########################################################################
> The doc string of `electric-indent-functions-without-reindent' says:
>     "List of indent functions that can't reindent."
> .  Even though the rest of the doc string explains what is meant, this
> top line is nonsensical - all the functions listed _can_ reindent.
> Better, I think, would be:
>     "List of indent functions which won't be used for reindentation."
> , even if not all that much better.  But what is meant by
> "REindentation", as opposed to "indentation"?

"Reindentation" is to adjust the indentation of an existing line.
It is premised on the idea that indent-line-function will "always" give
a result that's no worse than the current line's indentation.

In contrast, "indentation" in this context is when space is added on
a line that is not pre-existing, so in a sense it can't be worse than
what was there before, since there was nothing before.

Think of "reindent-then-newline-and-indent".

> Also, in `electric-indent-post-self-insert-function', there are two
> calls to `indent-according-to-mode'.  `e-i-f-without-reindent' is only
> checked for one of these calls.  Is this a bug?

There are also two calls in "reindent-then-newline-and-indent", the
first is the "reindent", then other is the "indent".

> ########################################################################
> In `electric--after-char-pos', there is the strange looking form:
>     (eq (char-before) last-command-event) ;; Sanity check.
> .  What is this supposed to check?  After inserting a newline,
> (char-before) is 10.  `last-command-event' is (on my Linux tty) either
> 10 or 13 (after typing C-j or <ret>).  Distingushing them here doesn't
> seem to make sense.

When the code is run last-command-event should be 10 in both cases:
`newline' let-binds last-command-event to 10.

>     What is this form intended to distinguish?

electric--after-char-pos' is all about finding the self-inserted char,
which is not always right before point, since abbrev-expansion and
post-self-insert-hook functions may have made arbitrary changes since
the char was inserted.

> #########################################################################
> Assuming the above meaning for `electric-indent-inhibit', then there is
> the following problem: even with `e-i-inhibit' set to t, electric
> indentation gets done on the new line after insertion of a \n.

If you don't want that, then I suggest you disable electric-indent-mode.

> regardless of `electric-indent-inhibit'.  This is surely a bug.

No, it's by design.  Otherwise electric-indent-inhibit would just disable
electric-indent-mode, which would be redundant: you can already do that
by ... turning it off.

> #########################################################################
> In general, `electric-mode-post-self-insert-function' seems horrifically
> and needlessly over-complicated, even though it is only 50 lines long.
> The various checks performed before invoking `indent-according-to-mode'
> are done at many different places at several different levels of nesting
> in the code.  For instance, why is `electric-indent-inhibit' checked
> twice at a lower level, rather than just once at the top level?
> Why can the various checks not simply be successive arms of an `and'
> form?

I wrote it as well as I could.  If you can make it simpler without
breaking the behavior, please do so.

Some of the interesting cases come up when it gets combined with things
like electric-pair-mode or electric-layout-mode.


        Stefan