stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [patch] Child process stats in exec utility (unix)
Date Thu, 14 Sep 2006 22:24:05 GMT
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