Package: coreutils;
Reported by: Jim Meyering <jim <at> meyering.net>
Date: Mon, 31 May 2010 11:04:01 UTC
Severity: wishlist
Tags: notabug
Message #14 received at 6318 <at> debbugs.gnu.org (full text, mbox):
From: Jim Meyering <jim <at> meyering.net> To: Paul Eggert <eggert <at> CS.UCLA.EDU> Cc: 6318 <at> debbugs.gnu.org, uncrustify-developer <at> lists.sourceforge.net Subject: Re: bug#6318: reindenting with uncrustify, maybe... Date: Wed, 02 Jun 2010 22:28:46 +0200
Paul Eggert wrote:
> On 05/31/2010 04:03 AM, Jim Meyering wrote:
>> I'm considering whether to format coreutils' sources using uncrustify.
>
> I tried your .uncrustify.cfg out on diffutils and ran into the
> following issues. Each issue is some diff output, followed by my
> comments on that diff. Diffutils is a different program, but I figure
> the suggestion would come up eventually there, too. Many of these
> issues are minor annoyances, but some of them are a bit more serious.
Hi Paul,
Thanks for the detailed feedback.
> @@ -1510,9 +1509,15 @@ output_diff3_edscript (FILE *outputfile,
> switch (type)
> {
> default: continue;
> - case DIFF_2ND: if (!show_2nd) continue; conflict = true; break;
> - case DIFF_3RD: if (overlap_only) continue; conflict = false; break;
> - case DIFF_ALL: if (simple_only) continue; conflict = flagging; break;
> + case DIFF_2ND: if (! show_2nd)
> + continue;
> + conflict = true; break;
> + case DIFF_3RD: if (overlap_only)
> + continue;
> + conflict = false; break;
> + case DIFF_ALL: if (simple_only)
> + continue;
> + conflict = flagging; break;
> }
>
> low0 = D_LOWLINE (b, mapping[FILE0]);
>
> Ouch! This is a complete botch of reindenting.
Maybe a bug. Or maybe there's an option to force a newline after
a case statement's ":", and we just need to find it and turn it on.
> ----------------------------------------------------------------
>
> #if HAVE_SIGACTION
> - /* Prefer `sigaction' if available, since `signal' can lose signals. */
> - static struct sigaction initial_action[NUM_SIGS];
> +/* Prefer `sigaction' if available, since `signal' can lose signals. */
> +static struct sigaction initial_action[NUM_SIGS];
> # define initial_handler(i) (initial_action[i].sa_handler)
> - static void signal_handler (int, void (*) (int));
> +static void signal_handler (int, void (*)(int));
> #else
> - static void (*initial_action[NUM_SIGS]) ();
> +static void (*initial_action[NUM_SIGS]) ();
> # define initial_handler(i) (initial_action[i])
> # define signal_handler(sig, handler) signal (sig, handler)
> #endif
>
> This is insisting on the style where preprocessor directives are
> indented independently of the non-preprocessor directives. But it's
> sometimes (as here) nice to use consistent indenting, for both
> directives and non-directives.
Would be nice, but how do we (not to mention the tool) know when it's desired?
> ----------------------------------------------------------------
>
> - mbstate_t mbstate = { 0 };
> + mbstate_t mbstate = {0};
>
> I don't see why the spaces were removed.
I could have gone either way, but there were far more cases
with no spaces, so I put this in the config file:
sp_inside_braces_struct = remove # Add or remove space inside '{' and '}'
sp_inside_braces = remove # Add or remove space inside '{' and '}'
> ----------------------------------------------------------------
>
> - while (changed0[i0]) ++i0;
> - while (changed1[i1]) ++i1;
> + while (changed0[i0])
> + ++i0;
> + while (changed1[i1])
> + ++i1;
>
> The old version is easier to read, since one can visually align the
> bodies of the two loops.
I agree that sometimes the one-line version is desired.
coreutils' sort.c also has code like that would be similarly
degraded if we were to convert today.
I hope we can arrange something.
uncrustify's code seems readable and maintainable enough that
if something needs to be changed and we're motivated enough,
we can do it ourselves.
> ----------------------------------------------------------------
>
> @@ -496,7 +500,7 @@ diff_2_files (struct comparison *cmp)
> changes = 0;
>
> else
> - /* Scan both files, a buffer at a time, looking for a difference. */
> + /* Scan both files, a buffer at a time, looking for a difference. */
> {
> /* Allocate same-sized buffers for both files. */
> size_t lcm_max = PTRDIFF_MAX - 1;
>
> It's a bit weird to unindent the comment so that it's the same as
> the preceding 'else'.
>
> ----------------------------------------------------------------
>
> - for (l0 = p0, l1 = p1; (l = *l0) == *l1; l0++, l1++)
> + for (l0 = p0, l1 = p1; (l = *l0) == *l1; l0++, l1++)
>
> I find the version with the extra spaces a bit easier to read,
> as the visual code-clumps match the semantics of the code.
> Perhaps "uncrustify" could be taught to preserve extra spaces
> in expressions when they match the expressions' semantics?
Sounds possible.
I wouldn't want to use two spaces all the time,
perhaps only when there are "," expressions in first and/or third term.
> ----------------------------------------------------------------
>
> - int offset_width IF_LINT (= 0);
> + int offset_width IF_LINT ( = 0);
>
> I don't see why that space was inserted.
I suppose it's because we're enforcing spaces around all such operators.
> ----------------------------------------------------------------
>
> - sprintf (buf, "%"PRIdMAX".%.9d", sec, nsec);
> + sprintf (buf, "%" PRIdMAX ".%.9d", sec, nsec);
>
> I'd rather not have those spaces inserted around the PRIdMAX.
Special case PRI* macros when adjacent to literal strings?
Another opportunity to teach uncrustify something new?
I'd welcome the feature, but its lack wouldn't motivate
me to add that capability.
> ----------------------------------------------------------------
>
> @@ -601,7 +604,7 @@ make_3way_diff (struct diff_block *threa
> /* Make the diff you just got info from into the using class */
> using[high_water_thread]
> = last_using[high_water_thread]
> - = high_water_diff;
> + = high_water_diff;
> current[high_water_thread] = high_water_diff->next;
> ...
> @@ -1156,7 +1157,7 @@ compare_files (struct comparison const *
> char const *fnm = cmp.file[fnm_arg].name;
> char const *dir = cmp.file[dir_arg].name;
> char const *filename = cmp.file[dir_arg].name = free0
> - = dir_file_pathname (dir, last_component (fnm));
> + = dir_file_pathname (dir, last_component (fnm));
>
> if (STREQ (fnm, "-"))
> fatal ("cannot compare `-' to a directory");
> @@ -1178,11 +1179,11 @@ compare_files (struct comparison const *
> /* Neither file "exists", so there's nothing to compare. */
> }
> else if ((same_files
> - = (cmp.file[0].desc != NONEXISTENT
> - && cmp.file[1].desc != NONEXISTENT
> - && 0 < same_file (&cmp.file[0].stat, &cmp.file[1].stat)
> - && same_file_attributes (&cmp.file[0].stat,
> - &cmp.file[1].stat)))
> + = (cmp.file[0].desc != NONEXISTENT
> + && cmp.file[1].desc != NONEXISTENT
> + && 0 < same_file (&cmp.file[0].stat, &cmp.file[1].stat)
> + && same_file_attributes (&cmp.file[0].stat,
> + &cmp.file[1].stat)))
> && no_diff_means_no_output)
> {
> /* The two named files are actually the same physical file.
>
> These changes of indentation are all bizarre; the old indentation is
> nicer.
>
> ----------------------------------------------------------------
>
> files_can_be_treated_as_binary =
> (brief & binary
> - & ~ (ignore_blank_lines | ignore_case | strip_trailing_cr
> - | (ignore_regexp_list.regexps || ignore_white_space)));
> + & ~(ignore_blank_lines | ignore_case | strip_trailing_cr
> + | (ignore_regexp_list.regexps || ignore_white_space)));
>
> It was odd to see lots of places where space was inserted after "!"
> and unary "-", but then a space was _removed_ after "~". Why the
> inconsistency?
Here's the config setting that handles space after "!":
# Add space after the '!' (not) operator
# Currently this mangles "!!sym" into "! ! sym".
sp_not = add
I suspect that it'd be easy to add sp_tilde to
have the same effect for "~".
> ----------------------------------------------------------------
>
> int
> diff_dirs (struct comparison const *cmp,
> - int (*handle_file) (struct comparison const *,
> - char const *, char const *))
> + int (*handle_file)(struct comparison const *,
> + char const *, char const *))
> {
> struct dirdata dirdata[2];
> int volatile val = EXIT_SUCCESS;
> ...
> - int v1 = (*handle_file) (cmp,
> - 0 < nameorder ? 0 : *names[0]++,
> - nameorder < 0 ? 0 : *names[1]++);
> + int v1 = (*handle_file)(cmp,
> + 0 < nameorder ? 0 : *names[0]++,
> + nameorder < 0 ? 0 : *names[1]++);
>
> ----------------------------------------------------------------
>
> The GNU style is to have a space between the function and
> the opening parenthesis before the 1st argument. Please
> don't remove that space.
That may be a bug.
I haven't investigated it yet.
I have made notes about similar problems in coreutils:
# FIXME: teach uncrustify not to do this:
# - int (*cmp) (char const *, char const *))
# + int (*cmp)(char const *, char const *))
# There are similar in pr.c:
# - (p->char_func) (' ');
# + (p->char_func)(' ');
> ----------------------------------------------------------------
>
> - for (i = *bucket; ; i = eqs[i].next)
> - if (!i)
> + for (i = *bucket;; i = eqs[i].next)
> + if (! i)
>
> Changing "; ;" to ";;" is questionable.
I've just added this to my ~/.uncrustify.cfg, and it appears to do
part of what you want by leaving one space between the adjacent semicolons.
sp_before_semi_for_empty = add
> ----------------------------------------------------------------
>
> static int const sigs[] = {
> #ifdef SIGHUP
> - SIGHUP,
> + SIGHUP,
> #endif
> #ifdef SIGQUIT
> - SIGQUIT,
> + SIGQUIT,
> #endif
> #ifdef SIGTERM
> - SIGTERM,
> + SIGTERM,
> #endif
>
> The old indentation made the identifiers line up better.
Agreed, but I've resigned myself to making a few compromises,
and so far the price looks right, once we get a few of these
nits ironed out.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.