bug#6318: reindenting with uncrustify, maybe...

[Jim Meyering]

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.