incubator-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] exec utility display subsystem
Date Mon, 28 Aug 2006 01:17:44 GMT
Andrew Black wrote:
> Greetings all.
> 
> Attached is a patch plus a pair of new files (to be placed in the util 
> subdirectory) to create a display subsystem for the exec utility.  This 
> is a preparatory patch for other changes related to displaying the results.
> 
> It may make sense to rename the existing output subsystem (as parser?) 
> and call the new subsystem 'output'.

I think parse.cpp (as opposed to parser.cpp) might be a better name
than output.cpp. Calling the file that handles the formatting of the
program output output.cpp (or maybe format.cpp?) sounds good.

We should also think about restructuring the util directory so as
to avoid file name collisions between the locale utilities and the
exec utility.

[...]
> 
> /************************************************************************
>  *
>  * exec.h - Interface declaration for the result display subsystem

This should be display.h (output.h or format.h after renaming).

[...]
> extern enum display_modes {
>     DISPLAY_PLAIN = 0, /**< plain text output, intended for files. */
>     DISPLAY_XTERM, /**< xterm colored output. */
>     DISPLAY_INDEP, /**< Non-targeted formated output. */
>     DISPLAY_HTML /**< HTML formated output. */
> } output_fmt;

I like the name output_fmt. How about naming the struct OutputFmt
and the enumerations FMT_XXX?

> 
> /**
>    Structure encapsulating the results extracted from a run.
> */
> struct target_status {
>     int exit; /**< exit code for process */
>     const char* status; /**< Name for process status */

Let's change this to an enum and let the formatting subsystem decide
how to format it.

>     unsigned warn; /**< Number of (test) warnings */
>     unsigned assert; /**< Number of (test) assertions */
>     unsigned failed; /**< Number of failed (test) assertions */
> };
> 
[...]
> void set_header (const char* format);
[...]
> void print_header ();
[...]
> void begin_row ();
[...]
> void end_row (const struct target_status* status);
[...]
> void print_footer ();

The function names here make the assumption that the output will
be presented in the format of a table even though that's not always
necessarily going to be the case (I would like to have a verbose
line oriented output with each executable producing several lines
of output -- see http://tinyurl.com/gb2nn).

So I think a better approach might be to focus on what is being
formatted rather than how. Something like:

     void fmt_target (const char *pathname, char **argv);
     void fmt_status (struct target_status *status);

fmt_target would be responsible for formatting the name of the
executable file (possibly including the command line arguments
passed to it, depending on the selected format), and fmt_status
would format the result of executing it (e.g., the exit status
followed by the numbers of warnings, assertions, etc.)

To make adding a new output format easy while avoiding having to
make changes to existing formatting functions, the fmt_target and
fmt_status functions should really be *function pointers* set to
point to the format-specific implementation functions. E.g., for
HTML, we might have

     void fmt_target_html (const char *pathname, char **argv);
     void fmt_status_html (struct target_status *status);

and set

     fmt_target = fmt_target_html;
     fmt_status = fmt_status_html;

in response to an --html command line option (or its equivalent).

With fmt_target_html and fmt_status_html living in a separate file
from, say fmt_target_text and fmt_status_text (and other formatting
functions) making changes to one format would avoid any risk of
breaking any of the other formats.

Btw., it occurs to me that it might be profitable to introduce
a new struct to represent each executable file along with its
full pathname, its argv array, and the other data we currently
store in struct target_status. That way each formatting function
would only need to take the struct as its argument and fmt_status
would also have access to other pieces of the data besides the
exit status and the warning and assertion counters.

[...]
> @@ -49,27 +50,23 @@
>     
>     This method tries to open out_name.  If this succedes, it then searches
>     the file for a result summary as produced by the rwtest driver.
> -   If a result summary is found, it then parses the result and displays the
> -   result of the parsing.  Otherwise, and appropriate error messages is 
> -   displayed.
> +   If a result summary is found, it then parses it and fills the status 
> +   structure with the results of the parsing.  Otherwise, status->status is 
> +   set to the error state.

The formatting of help output should probably be made part of the
output formatting module so that we can avoid hardcoding the code
strings.

[...]
> @@ -110,10 +107,10 @@
>                     expected to be active, so invert the total */
>                  if (8 == r_lvl && 0 == strcmp (target_name, "0.new"))
>                      r_active = r_total-r_active;
> -                failed += r_active;
> -                asserts += r_total;
> -                if (failed < r_active || asserts < r_total) {
> -                    puts (" OFLOW");
> +                status->failed += r_active;
> +                status->assert += r_total;
> +                if (status->failed < r_active || status->assert < r_total)
{
> +                    status->status = "OFLOW";

I'd change this (and all other similar assignments) to

     status->status = ST_OFLOW;

Martin

Mime
View raw message