stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: runall.sh replacement
Date Tue, 25 Jul 2006 18:30:30 GMT
Andrew Black wrote:
> Greetings Martin
> 
> Attached is a patch that tries to address the formatting issues 
> remaining.  I'm still not certain I've caught everything, but it's a 
> starting point.

Thanks! This is better but a) I think we should fix all the issues
before moving on, and b) as a matter of policy, we should fix only
formatting issues and nothing else in the patch. Could you please
resubmit just the formatting changes alone? A few more comments
below.

[...]
> @@ -235,14 +237,14 @@
>      size_t i;
>      static char def[16];
>  
> -    for (i = 0; i != sizeof names / sizeof *names; ++i) {
> +    for (i = 0; i < sizeof (names) / sizeof (*names); ++i) {

FYI: the parentheses are unnecessary here. The sizeof operator
requires parentheses around types but not around objects.

[...]
> @@ -548,29 +550,29 @@
>  
>          /* Set process group ID (so entire group can be killed)*/
>          {
> -            const pid_t pgroup = setsid();
> -            if ( getpid() != pgroup )
> -                terminate ( 1, "Error setting process group: %s\n",
> -                            strerror (errno));
> +            const pid_t pgroup = setsid ();
> +            if ( getpid () != pgroup )

This should be:

                if (getpid () != pgroup)


> +                terminate (1, "Error setting process group: %s\n",
> +                           strerror (errno));
[...]
> @@ -587,23 +589,23 @@
>                               S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  
>              if (-1 == intermit)
> -                terminate( 1,  "Error opening %s for output redirection: "
> -                           "%s\n", tmp_name, strerror (errno));
> +                terminate(1,  "Error opening %s for output redirection: "
> +                          "%s\n", tmp_name, strerror (errno));

The above is missing a space before the paren and has an extra
space after the comma. Btw., it's easy to write an Emacs regular
expression to correct this type of fomatting:

     search for: "\([a-z_A-Z0-9]\)("

     and replace it with: "\1 ("

You might want to consider doing something similar for the extra
space after the comma and the missing space before an opening
brace, since those two seem to be a recurring problem as well.

>  
> -            replace_file(intermit, 1, "stdout");
> +            replace_file (intermit, 1, "stdout");
>  
>              free (tmp_name);
>          }
>  
>          /* Redirect stderr */
> -        if (-1 == dup2(1, 2))
> -            terminate ( 1,  "Redirection of stderr to stdout failed: %s\n", 
> -                        strerror (errno));
> +        if (-1 == dup2 (1, 2))
> +            terminate (1,  "Redirection of stderr to stdout failed: %s\n", 

Extra space after the comma above :)

[...]
> @@ -611,11 +613,11 @@
>      if (-1 == child_pid){

Missing space before the closing brace. It's easy to fix, too:

   replace: "\([^ ]\){"
   with: "\1 {"

[...]
> --- output.cpp	(revision 425396)
> +++ output.cpp	(working copy)
> @@ -69,46 +69,36 @@
>         Parsed results
>  
[...]
>          case '#':
> -            fsm = ( 1 == fsm ) ? 2 : 0;
> +            fsm = (1 == fsm) ? 2 : 0;

FYI: There is no need to parenthesize the subexpressions in the
conditional expression.

>              break;
>          case ' ':
> -            fsm = ( 2 == fsm ) ? 3 : ( ( 4 == fsm ) ? 5 : 0 );
> +            fsm = (2 == fsm) ? 3 : ((4 == fsm) ? 5 : 0);

Same here. Personally, I find the whole expression more readable
without the parentheses (YMMV):

               fsm = 2 == fsm ? 3 : 4 == fsm ? 5 : 0;

[...]
>  /**
> @@ -159,30 +147,19 @@
>     this method and check_test() is how it parses the results.  This version
>     parses compatability layer output, rather than the test driver output.
>  
> -   @param exec_name the name of the executable that generated the output file
> -   @param out_name the name of the output file being parsed
> +   @param data pointer to file structure for output file being parsed
>     @see check_test()
>  */
>  static void
> -check_compat_test (const char* out_name)
> +check_compat_test (FILE* data)

FYI: it's a good policy to avoid making any other type of changes
when adjusting formatting. That way you reduce the likelihood of
introducing regressions and having to wade through hundreds of
lines of irrelevant and innocuous changes just to find it (or
worse yet, making someone else do it).

[...]
> -    assert ( 0 != out_name );
> +    assert ( 0 != data );

Extra spaces after the opening and before the closing parentheses.

>  
[...]
> @@ -272,7 +232,7 @@
>      struct stat file_info;
>      const size_t root_len = strlen(in_root);
>      char* const ref_name = (char*)RW_MALLOC(root_len 
> -                                           + strlen(exec_name) + 19);
> +                                            + strlen(exec_name) + 19);

Missing spaces before the opening parentheses above.

[...]
> @@ -210,7 +207,7 @@
>      assert (0 != path);
>  
>      for (mark = pos = path; '\0' != *(pos); ++pos)
> -        mark = ( '/' == *(pos) || '\\' == *(pos) ) ? pos+1 : mark;
> +        mark = ('/' == *(pos) || '\\' == *(pos)) ? pos + 1 : mark;

FYI: there is no need to parenthesize the name of a variable before
dereferencing it. I.e., the above is the same as the (IMO) more
readable equivalent:

          mark = '/' == *pos || '\\' == *pos ? pos + 1 : mark;

Martin

Mime
View raw message