stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Black <abl...@roguewave.com>
Subject Re: runall.sh replacement
Date Tue, 25 Jul 2006 20:56:19 GMT
Greetings Martin

Attached is a second try, this time without the non-formatting changes

--Andrew Black

Martin Sebor wrote:
> 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