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.
--Andrew Black
Log:
2006-07-25 Andrew Black <[EMAIL PROTECTED]>
* cmdopt.cpp: Formatting cleanup.
* exec.cpp: Same.
* output.cpp: Same.
* runall.cpp: Same.
* util.cpp: Same.
Martin Sebor wrote:
Andrew Black wrote:
Revised patch attached.
One change that I made while working on the self-test logic was to
split the output parsing logic into parse_output.cpp/h. I suspect
you'll have a better name for the file. That was bundled into this
patch as I didn't take the time to back those changes out.
Okay, I made a number of changes and committed everything here:
http://svn.apache.org/viewvc?rev=425242&view=rev
One important change was removing the non-portable "-q" option
from the invocation of diff (it was causing problems on Solaris).
I would still like to see a patch addressing the formatting issues
I pointed out initially (http://tinyurl.com/mmqgv), preferably
before any other changes to the utility.
Thanks!
Martin
Index: exec.cpp
===================================================================
--- exec.cpp (revision 425396)
+++ exec.cpp (working copy)
@@ -43,14 +43,16 @@
#include "exec.h"
/**
- Status flag used to comunicate that an alarm has triggered
+ Status flag used to comunicate that an alarm has triggered.
+
+ Value is 0 when alarm hasn't been triggered or has been handled
+ Value is 1 when alarm has been triggered and hasn't been handled
+
@see handle_alrm
@see open_input
*/
-static int
-alarm_timeout; /* set to a non-zero value when a non-zero timeout expires */
+static int alarm_timeout;
-
/**
Translates a signal number into a human understandable name
@@ -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) {
if (names [i].val == signo) {
return names [i].str;
}
}
/* We've run out of known signal numbers, so use a default name */
- snprintf (def, sizeof(def), "SIG#%d", signo);
+ snprintf (def, sizeof (def), "SIG#%d", signo);
return def;
}
@@ -308,7 +310,7 @@
#else
int waitopts = WUNTRACED;
#endif
- assert ( 1 < child_pid );
+ assert (1 < child_pid);
/* Clear timeout */
alarm_timeout = 0;
@@ -320,7 +322,7 @@
act.sa_handler = handle_alrm;
sigaction (SIGALRM, &act, 0);
- if (timeout>0)
+ if (timeout > 0)
alarm (timeout);
while (1) {
@@ -384,13 +386,13 @@
}
else if (ECHILD == errno) {
/* should not happen */
- fprintf (stderr, "waitpid(%d) error: %s\n",
- (int)child_pid, strerror (errno));
+ fprintf (stderr, "%s: waitpid(%d) error: %s\n",
+ exe_name, (int)child_pid, strerror (errno));
}
else {
/* waitpid() error */
- fprintf (stderr, "waitpid(%d) error: %s\n",
- (int)child_pid, strerror (errno));
+ fprintf (stderr, "%s: waitpid(%d) error: %s\n",
+ exe_name, (int)child_pid, strerror (errno));
}
}
else if ((pid_t)0 == wait_pid) {
@@ -453,8 +455,8 @@
/* If the file exists (errno isn't ENOENT), exit */
if (ENOENT != errno)
- terminate ( 1, "open(%s) failed: %s\n", tmp_name,
- strerror (errno));
+ terminate (1, "open(%s) failed: %s\n", tmp_name,
+ strerror (errno));
/* Try in_root/tutorial/in/exec_name.in */
memcpy (tmp_name, in_root, root_len+1);
@@ -471,22 +473,22 @@
/* If the file exists (errno isn't ENOENT), exit */
if (-1 == intermit && ENOENT != errno)
- terminate ( 1, "open(%s) failed: %s\n", tmp_name,
- strerror (errno));
+ terminate (1, "open(%s) failed: %s\n", tmp_name,
+ strerror (errno));
- free(tmp_name);
+ free (tmp_name);
}
/* If we didn't find a source file, open /dev/null */
- intermit = open("/dev/null", O_RDONLY);
+ intermit = open ("/dev/null", O_RDONLY);
/* If we opened the file, return the descriptor */
if (0 <= intermit)
return intermit;
/* otherwise, print an error message and exit */
- terminate ( 1, "open(/dev/null) failed: %s\n", strerror (errno));
+ terminate (1, "open(/dev/null) failed: %s\n", strerror (errno));
return -1; /* silence a compiler warning */
}
@@ -510,13 +512,13 @@
result = dup2 (source, dest);
if (-1 == result)
- terminate ( 1, "redirecting to %s failed: %s\n", name,
- strerror (errno));
+ terminate (1, "redirecting to %s failed: %s\n", name,
+ strerror (errno));
result = close (source);
if (-1 == result)
- terminate ( 1, "closing source for %s redirection failed: %s\n",
- name, strerror (errno));
+ terminate (1, "closing source for %s redirection failed: %s\n",
+ name, strerror (errno));
}
/**
@@ -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 )
+ terminate (1, "Error setting process group: %s\n",
+ strerror (errno));
}
/* Cache stdout for use if execv() fails */
{
const int error_cache = dup (2);
if (-1 == error_cache)
- terminate ( 1, "Error duplicating stderr: %s\n",
- strerror (errno));
+ terminate (1, "Error duplicating stderr: %s\n",
+ strerror (errno));
error_file = fdopen (error_cache,"a");
if (0 == error_file)
- terminate ( 1, "Error opening file handle from cloned "
- "stderr file descriptor: %s\n", strerror (errno));
+ terminate (1, "Error opening file handle from cloned "
+ "stderr file descriptor: %s\n", strerror (errno));
}
/* Redirect stdin */
{
const int intermit = open_input (exec_name);
- replace_file(intermit, 0, "stdin");
+ replace_file (intermit, 0, "stdin");
}
/* Redirect stdout */
@@ -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));
- 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",
+ strerror (errno));
execv (argv [0], argv);
- fprintf (error_file, "execv (\"%s\", ...) error: %s\n",
- argv [0], strerror (errno));
+ fprintf (error_file, "%s: execv (\"%s\", ...) error: %s\n",
+ exe_name, argv [0], strerror (errno));
exit (1);
}
@@ -611,11 +613,11 @@
if (-1 == child_pid){
/* Fake a failue to execute status return structure */
struct exec_attrs state = {127, -1};
- fprintf (stderr, "Unable to create child process for %s: %s\n",
- argv[0], strerror (errno));
+ fprintf (stderr, "%s: Unable to create child process for %s: %s\n",
+ exe_name, argv[0], strerror (errno));
return state;
}
/* parent */
- return wait_for_child ( child_pid );
+ return wait_for_child (child_pid);
}
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp (revision 425396)
+++ cmdopt.cpp (working copy)
@@ -77,7 +77,7 @@
@see exe_opts
*/
int
-eval_options (const int argc, /* const */ char* const argv[])
+eval_options (const int argc, char* const argv[])
{
int i;
@@ -171,10 +171,11 @@
const char *pos;
char *target, *last;
char **table_pos, **argv;
+ const size_t optlen = strlen (exe_opts);
assert (0 != exe_opts);
- if (0 == strlen (exe_opts)) {
+ if (0 == optlen) {
/* Alloc a an index array to hold the program name */
argv = (char**)RW_MALLOC (2 * sizeof (char*));
@@ -183,15 +184,14 @@
return argv;
}
- table_pos = argv = (char**)RW_MALLOC (
- (strlen (exe_opts) + 5) * sizeof (char*) / 2);
- /* (strlen (exe_opts)+5)/2 is overkill for the most situations,
- but it is just large enough to handle the worst case scenario
- (worst case is similar to 'x y' or 'x y z', requiring lengths
- of 4 and 5 respectively.)
+ table_pos = argv = (char**)RW_MALLOC ((optlen + 5) * sizeof (char*) / 2);
+ /* (strlen (exe_opts)+5)/2 is overkill for the most situations, but it is
+ just large enough to handle the worst case scenario. The worst case
+ is a string similar to 'x y' or 'x y z', requiring array lengths of 4
+ and 5 respectively.
*/
- last = target = argv[1] = (char*)RW_MALLOC (strlen (exe_opts) + 1);
+ last = target = argv[1] = (char*)RW_MALLOC (optlen + 1);
/* Transcribe the contents, handling escaping and splitting */
for (pos = exe_opts; *pos; ++pos) {
Index: output.cpp
===================================================================
--- output.cpp (revision 425396)
+++ output.cpp (working copy)
@@ -69,46 +69,36 @@
Parsed results
@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
*/
static void
-check_test (const char* exec_name, const char* out_name)
+check_test (const char* exec_name, FILE* data)
{
- FILE* data = fopen (out_name, "r");
unsigned r_lvl = 0, r_active = 0, r_total = 0;
unsigned asserts = 0, failed = 0;
int fmt_ok = 0;
unsigned fsm = 0;
char tok;
- assert ( 0 != exec_name );
- assert ( 0 != out_name );
+ assert (0 != exec_name);
+ assert (0 != data);
- if (0 == data) {
- if (ENOENT != errno) {
- printf ("Error opening %s: %s\n", out_name, strerror (errno));
- return;
- }
- puts ("OUTPUT");
- return;
- }
-
for (tok = fgetc (data); fsm < 6 && !feof (data); tok = fgetc (data)) {
switch (tok) {
case '\n':
fsm = 1;
break;
case '#':
- fsm = ( 1 == fsm ) ? 2 : 0;
+ fsm = (1 == fsm) ? 2 : 0;
break;
case ' ':
- fsm = ( 2 == fsm ) ? 3 : ( ( 4 == fsm ) ? 5 : 0 );
+ fsm = (2 == fsm) ? 3 : ((4 == fsm) ? 5 : 0);
break;
case '|':
- fsm = ( 3 == fsm ) ? 4 : 0;
+ fsm = (3 == fsm) ? 4 : 0;
break;
case '(':
- if ( 5 == fsm ) {
+ if (5 == fsm) {
fseek (data, -6, SEEK_CUR);
fsm++;
break;
@@ -124,12 +114,12 @@
if (6 < r_lvl) {
/* The 0.new tests produces errors, and are all
expected to be active, so invert the total */
- if (8 == r_lvl && 0 == strcmp(exec_name,"0.new"))
- r_active = r_total-r_active;
+ if (8 == r_lvl && 0 == strcmp (exec_name,"0.new"))
+ r_active = r_total - r_active;
failed += r_active;
asserts += r_total;
if (failed < r_active || asserts < r_total) {
- puts(" OFLOW");
+ puts (" OFLOW");
return;
}
}
@@ -141,15 +131,13 @@
if (fmt_ok) {
unsigned pcnt = 0;
if (asserts) {
- pcnt = 100 * ( asserts - failed ) / asserts;
+ pcnt = 100 * (asserts - failed) / asserts;
}
- printf(" 0 %6d %6d %5d%%\n", asserts, failed, pcnt);
+ printf (" 0 %6d %6d %5d%%\n", asserts, failed, pcnt);
}
else {
- puts("FORMAT");
+ puts ("FORMAT");
}
-
- fclose(data);
}
/**
@@ -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)
{
- FILE* data = fopen (out_name, "r");
unsigned asserts = 0, failed = 0;
int read = 0;
unsigned fsm = 0;
char tok;
- assert ( 0 != out_name );
+ assert ( 0 != data );
- if (0 == data) {
- if (ENOENT != errno) {
- printf ("Error opening %s: %s\n", out_name, strerror (errno));
- return;
- }
- puts ("OUTPUT");
- return;
- }
-
fseek (data, -64, SEEK_END); /* Seek near the end of the file */
for (tok = fgetc (data); fsm < 4 && !feof (data); tok = fgetc (data)) {
@@ -206,9 +183,9 @@
}
}
- if (!feof(data)) { /* leading "## A" eaten above */
- read = fscanf(data, "ssertions = %u\n## FailedAssertions = %u",
- &asserts, &failed);
+ if (!feof (data)) { /* leading "## A" eaten above */
+ read = fscanf (data, "ssertions = %u\n## FailedAssertions = %u",
+ &asserts, &failed);
}
if (2 == read) {
@@ -221,25 +198,9 @@
else {
puts("FORMAT");
}
-
- fclose(data);
}
/**
- Sanity test macro for file descriptor operations.
-
- @killme this should be removed after removing the dependancy on the
- posix diff utility
-
- @param op human understandable name for operation
- @param x variable to test the result for
- @see check_example
-*/
-#define FILE_TEST(op, x) \
- if (-1==(x)) \
- terminate ( 2, op " failed: %s\n", strerror (errno) )
-
-/**
Parses output file out_name for the example exec_name.
This method tries to compare out_name against a reference output file and
@@ -263,7 +224,6 @@
@param exec_name the name of the executable that generated the output file
@param out_name the name of the output file being parsed
@see in_root
- @see FILE_TEST()
@todo remove dependancy on POSIX diff utility
*/
static void
@@ -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);
int state = -1;
assert (0 != in_root);
@@ -286,11 +246,11 @@
strcat (ref_name, exec_name);
strcat (ref_name, ".out");
- if (0 > stat(ref_name, &file_info)) {
+ if (0 > stat (ref_name, &file_info)) {
if (ENOENT != errno) {
- printf("stat(%s) error: %s\n", ref_name,
- strerror (errno));
- free(ref_name);
+ printf ("stat(%s) error: %s\n", ref_name,
+ strerror (errno));
+ free (ref_name);
return;
}
@@ -301,15 +261,13 @@
strcat (ref_name, exec_name);
strcat (ref_name, ".out");
- if (0 > stat(ref_name, &file_info)) {
- if (ENOENT != errno) {
- printf("stat(%s) error: %s\n", ref_name,
- strerror (errno));
- }
- else {
- puts(" NOREF");
- }
- free(ref_name);
+ if (0 > stat (ref_name, &file_info)) {
+ if (ENOENT != errno)
+ printf ("stat(%s) error: %s\n", ref_name,
+ strerror (errno));
+ else
+ puts (" NOREF");
+ free (ref_name);
return;
}
}
@@ -320,18 +278,24 @@
/* Cache stdout (hopefully) for use if execv() fails */
int error_cache = dup(2);
FILE* error_file;
- FILE_TEST("dup(stderr)", error_cache);
- FILE_TEST("close(stdin)",close(0));
- FILE_TEST("close(stdin)",close(1));
- FILE_TEST("close(stderr)",close(2));
+ if (-1 == error_cache)
+ terminate (1, "Error duplicating stderr: %s\n",
+ strerror (errno));
+ if (-1 == close(0))
+ terminate (1, "Error closing stdin: %s\n", strerror (errno));
+ if (-1 == close(1))
+ terminate (1, "Error closing stdout: %s\n", strerror (errno));
+ if (-1 == close(2))
+ terminate (1, "Error closing stderr: %s\n", strerror (errno));
+
/* Todo: diff with --strip-trailing-cr on windows */
execlp ("diff", "diff", ref_name, out_name, (char *)0);
if ((error_file = fdopen (error_cache, "a")))
- fprintf (error_file, "execlp(\"diff\", ...) error: %s\n",
- strerror (errno));
+ fprintf (error_file, "%s: execlp(\"diff\", ...) error: %s\n",
+ exe_name, strerror (errno));
exit(2);
}
@@ -345,18 +309,18 @@
const int retcode = WEXITSTATUS (state);
switch ( retcode ) {
case 0:
- puts(" 0");
+ puts (" 0");
break;
case 1:
- puts("OUTPUT");
+ puts ("OUTPUT");
break;
default:
- printf("diff returned %d\n", retcode);
+ printf ("diff returned %d\n", retcode);
}
break;
}
else if (WIFSIGNALED (state)) {
- printf("diff exited with %s\n",
+ printf ("diff exited with %s\n",
get_signame (WTERMSIG (state)));
break;
}
@@ -386,17 +350,30 @@
void
parse_output (const char* target, const char* exec_name)
{
- const size_t path_len = strlen ( target );
- char* const out_name = (char*)RW_MALLOC ( path_len + 5);
+ const size_t path_len = strlen (target);
+ char* const out_name = (char*)RW_MALLOC (path_len + 5);
memcpy (out_name, target, path_len + 1);
strcat (out_name,".out");
if (!strlen (in_root)) {
/* If there is not an input directory, look at the assertion tags */
- if (!compat)
- check_test (exec_name, out_name);
- else
- check_compat_test (out_name);
+
+ FILE* data = fopen (out_name, "r");
+
+ if (0 == data) {
+ if (ENOENT != errno)
+ printf ("Error opening %s: %s\n", out_name, strerror (errno));
+ else
+ puts ("OUTPUT");
+ }
+ else{
+ if (!compat)
+ check_test (exec_name, data);
+ else
+ check_compat_test (data);
+
+ fclose(data);
+ }
}
else{
/* Otherwise, diff against the output file */
Index: util.cpp
===================================================================
--- util.cpp (revision 425396)
+++ util.cpp (working copy)
@@ -28,6 +28,7 @@
#include <string.h> /* for strerror */
#include <sys/types.h> /* for size_t */
+#include "cmdopt.h" /* for exe_name */
#include "util.h"
/**
@@ -44,11 +45,13 @@
assert (0 != format);
assert (0 != state);
- va_start ( args, format );
- vfprintf ( stderr, format, args );
- va_end ( args );
+ fprintf (stderr, "%s: ", exe_name);
- exit ( state );
+ va_start (args, format);
+ vfprintf (stderr, format, args);
+ va_end (args);
+
+ exit (state);
}
/**
@@ -62,13 +65,13 @@
void*
guarded_malloc (const size_t size, const char* const file, const unsigned line)
{
- void* const alloc = malloc(size);
+ void* const alloc = malloc (size);
assert (0 != file);
assert (0 < size);
- if ( 0 == alloc )
- terminate ( 1, "malloc(%lu) at line %u of %s failed: %s\n",
+ if (0 == alloc)
+ terminate (1, "malloc(%lu) at line %u of %s failed: %s\n",
(unsigned long)size, line, file, strerror (errno));
return alloc;
Index: runall.cpp
===================================================================
--- runall.cpp (revision 425396)
+++ runall.cpp (working copy)
@@ -86,7 +86,7 @@
/* This is roughly equivlent to the -x bash operator.
It checks if the file can be run, /not/ if we can run it
*/
- const size_t path_len = strlen ( target );
+ const size_t path_len = strlen (target);
char* tmp_name;
/* If target is a .o file, check if it exists */
@@ -105,24 +105,21 @@
}
/* Otherwise, check for the .o file */
- tmp_name = (char*)RW_MALLOC ( path_len + 3 );
+ tmp_name = (char*)RW_MALLOC (path_len + 3);
memcpy (tmp_name, target, path_len + 1);
strcat (tmp_name,".o");
- if (0 > stat(tmp_name, &file_info)) {
- if (ENOENT != errno) {
- printf ("stat(%s) error: %s\n", tmp_name,
- strerror (errno));
- }
- else {
+ if (0 > stat (tmp_name, &file_info)) {
+ if (ENOENT != errno)
+ printf ("stat(%s) error: %s\n", tmp_name, strerror (errno));
+ else
puts (" COMP");
- }
}
else {
puts (" LINK");
}
- free(tmp_name);
+ free (tmp_name);
return 0;
}
return 1;
@@ -159,7 +156,7 @@
const struct exec_attrs* result)
{
if (0 == result->status) {
- parse_output(target, exec_name);
+ parse_output (target, exec_name);
}
else if (WIFEXITED (result->status)) {
const int retcode = WEXITSTATUS (result->status);
@@ -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;
return mark;
}
@@ -239,8 +236,8 @@
childargv [0] = target;
- printf ( "%-18.18s ", exec_name );
- fflush ( stdout );
+ printf ("%-18.18s ", exec_name);
+ fflush (stdout);
if ( !check_target_ok (target))
return;
@@ -264,7 +261,7 @@
int
main (int argc, char *argv[])
{
- exe_name = argv[0];
+ exe_name = argv [0];
if (1 < argc && '-' == argv [1][0]) {
const int nopts = eval_options (argc, argv);
@@ -285,7 +282,7 @@
char** childargv = split_child_opts();
assert ( 0 != childargv );
- puts("NAME STATUS ASSRTS FAILED PERCNT");
+ puts ("NAME STATUS ASSRTS FAILED PERCNT");
for ( i = 0; i < argc; ++i ) {
run_target ( argv [i], childargv );
