incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Black <abl...@roguewave.com>
Subject Re: [patch] Child process stats in exec utility (unix)
Date Thu, 14 Sep 2006 23:11:05 GMT
How does a tweaked patch (and changelog) sound instead of a follow up patch?

--Andrew Black

Log:
     * display.h (unistd.h) [!_WIN32 && !_WIN64]: Include.
       (sys/time.h) [_XOPEN_UNIX]: Include.
       (rw_time_t, rw_suseconds_t, struct rw_timeval) [!_XOPEN_UNIX]: 
Define placeholder structures.
       (rw_timeval): Define abstraction typedef.
       (struct target_status): Add run, user, sys elements.
     * display.cpp (unistd.h) [!_WIN32 && !_WIN64]: Include.
       print_header_plain: Alter cols for process times.
       print_target_plain: Partition output column printing by section, 
add timing output.
     * exec.h (exec_file): Alter signature to accept target_status 
rather than char**.
     * exec.cpp (display.h): Include.
       (calculate_usage) [!_WIN32 && !_WIN64]: Define function to 
populate user and sys fields of provided target_status struct (if 
_XOPEN_UNIX is defined).
       (exec_file): Alter to accept target_status rather than char**, 
use argv element in place of old char** argument.
       (exec_file) [!_WIN32 && !_WIN64]: call calculate_usage after 
wait_for_child.
     * runall.cpp (run_target): Pass target_status struct rather than 
argv element.


Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings all
>>
>> Attached is the revised version of this patch, now that the display 
>> subsystem has been implemented.
> 
> This looks pretty good. One comment: it's good to avoid preprocessor
> conditionals as much as possible so that the same code gets compiled
> regardless of the platform (smaller chance of breakage that way).
> (Note that in library development this rule is secondary to space
> and speed efficiency.)
> 
> So...
> 
> [...]
>> Index: exec.cpp
>> ===================================================================
>> --- exec.cpp    (revision 443137)
>> +++ exec.cpp    (working copy)
> [...]
>> @@ -690,6 +691,60 @@
>>          }
>>      }
>>  }
> [...]
>> +static void
>> +calculate_usage (struct target_status* result)
> 
> ...this function should be defined and called regardless of whether
> or not it knows how to compute the usage (it could be empty when it
> doesn't), and its caller(s)...
> 
> [...]
>> @@ -708,15 +763,17 @@
>>     @see wait_for_child ()
>>  */
>>  struct exec_attrs -exec_file (char** argv)
>> +exec_file (struct target_status* result)
> [...]
>> @@ -783,13 +840,17 @@
>>      if (-1 == child_pid) {
>>          /* Fake a failue to execute status return structure */
>>          struct exec_attrs state = {127, -1};
>> -        warn ("Unable to create child process for %s: %s\n", argv [0],
>> +        warn ("Unable to create child process for %s: %s\n", 
>> result->argv [0],
>>                strerror (errno));
>>          return state;
>> +    } else {
>> +        /* parent */
>> +        struct exec_attrs state = wait_for_child (child_pid);
>> +#ifdef _XOPEN_UNIX
>> +        calculate_usage (result);
>> +#endif /* _XOPEN_UNIX */
> 
> ...should call it unconditionally and should be prepared for it
> not to be able to produce meaningful values.
> 
> [...]
>> Index: display.cpp
>> ===================================================================
>> --- display.cpp    (revision 443137)
>> +++ display.cpp    (working copy)
>> @@ -26,7 +26,14 @@
>>  
>>  #include <assert.h>
>>  #include <stdio.h>      /* for fflush(), printf(), puts(), ... */
>> +#if !defined (_WIN32) && !defined (_WIN64)
>> +#  include <unistd.h> /* for _XOPEN_UNIX */
>> +#endif
>>  
>> +#ifdef _XOPEN_UNIX
>> +#  define CHILD_STATS
>> +#endif
>> +
>>  #include "cmdopt.h" /* for target_name -should this be moved? */
>>  #include "exec.h" /* for get_signame */
>>  
>> @@ -34,7 +41,12 @@
>>  
>>  void print_header_plain ()
>>  {
>> +#ifdef CHILD_STATS
>> +    puts ("NAME                      STATUS    USER     SYS ASSERTS 
>> FAILED "
>> +          "PERCNT");
>> +#else
>>      puts ("NAME                      STATUS ASSERTS FAILED PERCNT");
>> +#endif
> 
> Similarly this function, instead of using the preprocessor, should
> take an argument telling it which of the two headers to print (or
> it could print the first one unconditionally), and...
> 
>> @@ -48,19 +60,31 @@
> [...]
>> +    /* Print timings, if available */
>> +#ifdef CHILD_STATS
>> +    if (status->run && ST_NOT_KILLED != status->status)
>> +        printf (" %3ld.%03ld %3ld.%03ld", status->user.tv_sec,
>> +                status->user.tv_usec/1000, status->sys.tv_sec,
>> +                status->sys.tv_usec/1000);
>> +    else if (status->assert)
>> +        printf ("                ");
>> +#endif
> 
> ...this call to printf should be guarded by a runtime conditional
> rather than a preprocessor one.
> 
> I'm fine committing the code as is as long as this patch is followed
> by one implementing the suggested changes sometime (very) soon  :)
> Let me know which way you want to proceed.
> 
> Thanks
> Martin

Mime
View raw message