Code cleanup.

Code cleanup.

[David Kastrup]

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


Hi,

in the course of checking the code for call-interactively (I have a
tendency to read code rather than the documentation), I felt a bit
like coming upon an obfuscated C contest.

I attach a patch (untested though) that does nothing except hopefully
get a more readable code.

Ok, so we are in freeze anyway, so tampering with any code that works
by replacing it with code designed to be completely equivalent is
probably not worth the potential trouble.

At what stage of development (if at all) should such janitorial
changes be usually applied?

And are there others that would agree with my assessment concerning
the readability here, or that would have even better ideas how this
would be expressed more obvious?

On a completely different tangent: wouldn't it be much more readable
(though likely not completely correct in some perverse manner) if
diff-mode actually had its TAB positions in column 9,17,25... instead
of the customary 8,16,24...?  At least in context and unified diffs,
that would _much_ better reflect the relative indentation of the
actual change than the current behavior.

It does not look like `tab-width' can be made to do that, but maybe
one could use a display property in order to move the information of
column 0 into the fringe.

But it would probably be saner if tab-width were extended to allow,
say, a cons-cell of initial offset and tab-width.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1013 bytes --]

*** callint.c	19 Oct 2006 21:07:18 +0200	1.148
--- callint.c	23 Oct 2006 13:30:50 +0200	
***************
*** 473,488 ****
    /* Count the number of arguments the interactive spec would have
       us give to the function.  */
    tem = string;
!   for (j = 0; *tem; j++)
      {
        /* 'r' specifications ("point and mark as 2 numeric args")
  	 produce *two* arguments.  */
!       if (*tem == 'r') j++;
        tem = (unsigned char *) index (tem, '\n');
        if (tem)
! 	tem++;
        else
! 	tem = (unsigned char *) "";
      }
    count = j;
  
--- 473,491 ----
    /* Count the number of arguments the interactive spec would have
       us give to the function.  */
    tem = string;
!   for (j = 0; *tem;)
      {
        /* 'r' specifications ("point and mark as 2 numeric args")
  	 produce *two* arguments.  */
!       if (*tem == 'r')
! 	j += 2;
!       else
! 	j++;
        tem = (unsigned char *) index (tem, '\n');
        if (tem)
! 	++tem;
        else
! 	break;
      }
    count = j;
  

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Code cleanup.

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> At what stage of development (if at all) should such janitorial
> changes be usually applied?

Code cleanup is usually welcome -- but this is not the time.

> On a completely different tangent: wouldn't it be much more readable
> (though likely not completely correct in some perverse manner) if
> diff-mode actually had its TAB positions in column 9,17,25... instead
> of the customary 8,16,24...?  At least in context and unified diffs,
> that would _much_ better reflect the relative indentation of the
> actual change than the current behavior.
>
> It does not look like `tab-width' can be made to do that, but maybe
> one could use a display property in order to move the information of
> column 0 into the fringe.

Simpler would be to put the "diff decoration" in the left margin and
put the fringe outside the margins.  Then the user wouldn't see any
difference (except that tabs line up correctly).

> But it would probably be saner if tab-width were extended to allow,
> say, a cons-cell of initial offset and tab-width.

Or a list (1 9 17 t)  
(where t means to repeat tabs every "difference between last two elements).

--
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Code cleanup.

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> David Kastrup <dak@gnu.org> writes:
>
>> At what stage of development (if at all) should such janitorial
>> changes be usually applied?
>
> Code cleanup is usually welcome -- but this is not the time.

Sure.

>> On a completely different tangent: wouldn't it be much more
>> readable (though likely not completely correct in some perverse
>> manner) if diff-mode actually had its TAB positions in column
>> 9,17,25... instead of the customary 8,16,24...?  At least in
>> context and unified diffs, that would _much_ better reflect the
>> relative indentation of the actual change than the current
>> behavior.
>>
>> It does not look like `tab-width' can be made to do that, but maybe
>> one could use a display property in order to move the information
>> of column 0 into the fringe.
>
> Simpler would be to put the "diff decoration" in the left margin and
> put the fringe outside the margins.  Then the user wouldn't see any
> difference (except that tabs line up correctly).

Ah, I forgot the display margins.

>> But it would probably be saner if tab-width were extended to allow,
>> say, a cons-cell of initial offset and tab-width.
>
> Or a list (1 9 17 t)  
> (where t means to repeat tabs every "difference between last two
> elements).

Or a list (8 . #0) for repeated differences of 8, and (9 8 . #1) for
that of 9.  Too bad that this print syntax of
(let ((n (list 9 8))) (setcdr (cdr n) (cdr n)) n)
is not accepted by the Lisp reader.

Circular lists are not really the most natural Lisp constructs...

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Code cleanup.

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>> David Kastrup <dak@gnu.org> writes:
>
>>> But it would probably be saner if tab-width were extended to
>>> allow, say, a cons-cell of initial offset and tab-width.
>>
>> Or a list (1 9 17 t)  
>> (where t means to repeat tabs every "difference between last two
>> elements).
>
> Or a list (8 . #0) for repeated differences of 8, and (9 8 . #1) for
> that of 9.  Too bad that this print syntax of
> (let ((n (list 9 8))) (setcdr (cdr n) (cdr n)) n)
> is not accepted by the Lisp reader.
>
> Circular lists are not really the most natural Lisp constructs...

Well, but one could still allow a thing like '(1 9 17 . 8) namely have
the first CDR that is not a list indicate the offset to use from then
on.  In that way, just a single 8 will then "naturally" correspond to
the normal case, and '(1 . 8) would be fine for the diff case.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Code cleanup.

[Stefan Monnier]

> in the course of checking the code for call-interactively (I have a
> tendency to read code rather than the documentation), I felt a bit
> like coming upon an obfuscated C contest.

> I attach a patch (untested though) that does nothing except hopefully
> get a more readable code.

> Ok, so we are in freeze anyway, so tampering with any code that works
> by replacing it with code designed to be completely equivalent is
> probably not worth the potential trouble.

> At what stage of development (if at all) should such janitorial
> changes be usually applied?

> And are there others that would agree with my assessment concerning
> the readability here, or that would have even better ideas how this
> would be expressed more obvious?

I'd replace

      if (*tem == 'r')
	j += 2;
      else
	j++;

with
 
      j += (*tem == 'r' ? 2 : 1);

but overall, it's a bit better.  In any case, I'd rather move most of it
to elisp, adding an alist variable that maps letters to the
corresponding function.

> On a completely different tangent: wouldn't it be much more readable
> (though likely not completely correct in some perverse manner) if
> diff-mode actually had its TAB positions in column 9,17,25... instead
> of the customary 8,16,24...?  At least in context and unified diffs,
> that would _much_ better reflect the relative indentation of the
> actual change than the current behavior.

For unified diffs, yes.  But for plain context diffs like the one you sent,
it'd have to be 10, 18, 26, ... and for files mixing the two it'd have to
be.... oh no!

Of course, it could all be done with display properties added from
diff-font-lock-keywords (and work even for mixed diffs).


        Stefan

Re: Code cleanup.

[Richard Stallman]

The right time for code cleanup would be whenever we
are installing new features.

However, in my opinion, your change does not either increase
or decrease readability.  It's a tossup.

Re: Code cleanup.

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> The right time for code cleanup would be whenever we
> are installing new features.
>
> However, in my opinion, your change does not either increase
> or decrease readability.  It's a tossup.

Uh, setting tem to "", an artificial empty string, in order to have j
incremented once again before breaking out of the finished loop is
readable?

Is this kind of "readable" synonymous to "comprehensible with serious
effort", reminiscent of mathematicians' use of "trivial" as synonymous
with "provable with serious effort"?

If one hits a terminating condition of a loop, one should exit the
loop instead of setting up artificial values to variables that have
the only purpose of causing an exit at a convenient point later in
time.

In other words: while "readability" is certainly a matter of personal
taste, you find me surprised at your taste.

I don't claim that my proposal was perfect, but I would not have
thought it a tossup.

I'll keep my version in my private tree, and will likely sneak it in
after the release.  At least I hope that nobody will find my proposal
actually strictly less readable than the original, and so we'll at
least get one developer (myself) marginally more happy without
offending others.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum