Package: coreutils;
Reported by: Jim Meyering <jim <at> meyering.net>
Date: Mon, 31 May 2010 11:04:01 UTC
Severity: wishlist
Tags: notabug
Message #11 received at 6318 <at> debbugs.gnu.org (full text, mbox):
From: Paul Eggert <eggert <at> CS.UCLA.EDU> To: Jim Meyering <jim <at> meyering.net> 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 11:52:15 -0700
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.
@@ -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.
----------------------------------------------------------------
#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.
----------------------------------------------------------------
- mbstate_t mbstate = { 0 };
+ mbstate_t mbstate = {0};
I don't see why the spaces were removed.
----------------------------------------------------------------
- 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.
----------------------------------------------------------------
@@ -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?
----------------------------------------------------------------
- int offset_width IF_LINT (= 0);
+ int offset_width IF_LINT ( = 0);
I don't see why that space was inserted.
----------------------------------------------------------------
- sprintf (buf, "%"PRIdMAX".%.9d", sec, nsec);
+ sprintf (buf, "%" PRIdMAX ".%.9d", sec, nsec);
I'd rather not have those spaces inserted around the PRIdMAX.
----------------------------------------------------------------
@@ -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?
----------------------------------------------------------------
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.
----------------------------------------------------------------
- for (i = *bucket; ; i = eqs[i].next)
- if (!i)
+ for (i = *bucket;; i = eqs[i].next)
+ if (! i)
Changing "; ;" to ";;" is questionable.
----------------------------------------------------------------
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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.