GNU bug report logs - #6318
reindenting with uncrustify, maybe...

Previous Next

Full log

View this message in rfc822 format

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: 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.