I believe I posted my patch to the list prior to the referenced change
being submitted to subversion, but I am uncertain why those particular
hunks failed to apply (running svn up appears to have merged them
without conflicts).
Attached is a revised version of the patch that tries to address the
formatting issues in changed code (a formatting and comment cleanup
sweep is needed after the windows port is finished). I have also tried
to alter the log based on the changes to the patch and the referenced link.
--Andrew Black
Log:
* util.h (reference_name, output_name): Declare functions to
generate the names for reference and output files respectively.
* util.cpp (reference_name, output_name): Define above.
(guarded_malloc, guarded_realloc): Allocate memory after asserts.
* cmdopt.h (escape_code, default_path_sep, suffix_len, suffix_sep):
Declare (platform specific) file system related constants.
* cmdopt.cpp (escape_code, default_path_sep, suffix_len,
suffix_sep): Define above for UNIX systems.
(split_opt_string): Move use of opts after assert, use
escape_code as escape character in place of '\'.
* exec.cpp: (get_signame): Enlarge static buffer, use sprintf in
place of snprintf.
(open_input): Use reference_name to determine input file name,
remove root_len variable, move use of in_root after asserts.
(exec_file): Alter to use output_name to determine output file name.
* output.cpp (check_example): Use reference_name to determine
reference file location, add assert on on_name.
(parse_output): Use output_name to determine output file name.
* runall.cpp (merge_argv): Use target after asserts.
(check_target_ok): Disable (unused) logic for output only
targets. Alter compile check on windows systems to correctly locate
.obj file.
(rw_basename): Use default_path_sep as separator.
(run_target): Use target, argv, childargv after asserts.
Martin Sebor wrote:
Andrew Black wrote:
Ok...
Lets try splitting some of the issues brought up into a separate patch
(attached, log below). This patch focuses on the snprintf issue, the
use of asserts(), and centralizing filename generation logic.
Included is the file name token abstraction logic. The possibility
was raised that path separators may be more than one character long.
I would agree that it should be considered, but I also feel that to
code for this possibility would slow the program down slightly, for no
immediate benefit, as all current target platforms seem to use a
single character as the directory separator.
Andrew, I'm afraid this patch doesn't apply cleanly -- see below.
I suspect you didn't integrate the following change (output.cpp.rej
is attached): http://svn.apache.org/viewvc?view=rev&revision=431269.
$ patch -d /build/sebor/stdcxx/util/ < ~/tmp/winprep2.diff
Looks like a unified context diff.
The next patch looks like a unified context diff.
The next patch looks like a unified context diff.
The next patch looks like a unified context diff.
Hunk #1 failed at line 232.
Hunk #2 failed at line 242.
2 out of 4 hunks failed: saving rejects to output.cpp.rej
The next patch looks like a unified context diff.
The next patch looks like a unified context diff.
The next patch looks like a unified context diff.
done
Could you please fix that and resubmit the patch?
Also, I would really like you to correct the little formatting
problems that I pointed out in my first feedback. I will not
commit the patch until these have been fixed. I believe a
consistent formatting style is important to the readability
of code and hence to its maintainability and I would rather
not spend time reviewing another round of changes just to
clean up these trivial problems.
--Andrew Black
Log:
* util.h (reference_name, output_name): Declare functions to
generate the names for reference and output files respectively.
* util.cpp (reference_name, output_name): Define above.
* util.cpp (guarded_malloc, guarded_realloc): move use of size
asserts
Please do not repeat the file name or the function name part on
subsequent lines. It makes it look like you changed more files
than you actually did.
You might want to give the Change Logs section of the GNU coding
standards a read:
http://www.gnu.org/prep/standards/html_node/Change-Logs.html
Thanks
Martin
Index: util.h
===================================================================
--- util.h (revision 431602)
+++ util.h (working copy)
@@ -52,4 +52,24 @@
void* guarded_realloc (void* source, const size_t size,
const char* const file, const unsigned line);
+/**
+ Generates the name of a reference (input/output) file, based on dir and mode.
+
+ This function allocates memory which is to be freed by the caller.
+
+ @param dir example subdirectory to reference
+ @param mode type of file to generate name for (should be 'in' or 'out')
+ @return translation of 'in_root/dir/mode/target_name.mode'
+*/
+char* reference_name (const char* dir, const char* mode);
+
+/**
+ Generates the name of the output file for the executable target.
+
+ This function allocates memory which is to be freed by the caller.
+
+ @param path of target to generate output name for
+ @return translation of 'target.out'
+*/
+char* output_name (const char* target);
#endif // RW_UTIL_H
Index: exec.cpp
===================================================================
--- exec.cpp (revision 431602)
+++ exec.cpp (working copy)
@@ -341,7 +341,7 @@
get_signame (int signo)
{
size_t i;
- static char def [16];
+ static char def [32];
for (i = 0; signal_names [i].str; ++i) {
if (signal_names [i].val == signo) {
@@ -350,7 +350,7 @@
}
/* We've run out of known signal numbers, so use a default name */
- snprintf (def, sizeof def, "SIG#%d", signo);
+ sprintf (def, "SIG#%d", signo);
return def;
}
@@ -535,22 +535,16 @@
static int
open_input (const char* exec_name)
{
- const size_t root_len = strlen (in_root);
int intermit = -1;
assert (0 != exec_name);
assert (0 != in_root);
- if (root_len) {
- const size_t out_len = root_len + strlen (exec_name) + 17;
-
- char* const tmp_name = (char*)RW_MALLOC (out_len);
+ if (strlen (in_root)) {
+ char* tmp_name;
/* Try in_root/manual/in/exec_name.in */
- memcpy (tmp_name, in_root, root_len+1);
- strcat (tmp_name, "/manual/in/");
- strcat (tmp_name, exec_name);
- strcat (tmp_name, ".in");
+ tmp_name = reference_name ("manual", "in");
intermit = open (tmp_name, O_RDONLY);
/* If we opened the file, return the descriptor */
@@ -565,10 +559,8 @@
strerror (errno));
/* Try in_root/tutorial/in/exec_name.in */
- memcpy (tmp_name, in_root, root_len+1);
- strcat (tmp_name, "/tutorial/in/");
- strcat (tmp_name, exec_name);
- strcat (tmp_name, ".in");
+ free (tmp_name);
+ tmp_name = reference_name ("tutorial", "in");
intermit = open (tmp_name, O_RDONLY);
/* If we opened the file, return the descriptor */
@@ -683,14 +675,9 @@
/* Redirect stdout */
{
- const size_t exelen = strlen (argv [0]);
- const size_t outlen = exelen + 5;
- char* const tmp_name = (char*)RW_MALLOC (outlen);
+ char* const tmp_name = output_name (argv [0]);
int intermit;
- /* Redirect stdout */
- memcpy (tmp_name, argv [0], exelen + 1);
- strcat (tmp_name, ".out");
intermit = open (tmp_name, O_WRONLY | O_CREAT | O_TRUNC,
S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp (revision 431602)
+++ cmdopt.cpp (working copy)
@@ -50,6 +50,10 @@
const char* in_root = ""; /**< Root directory for input/reference files. */
const char* exe_name; /**< Alias for process argv [0]. */
const char* target_name;
+const char escape_code = '\\';
+const char default_path_sep = '/';
+const char suffix_sep = '.';
+const size_t exe_suffix_len = 0;
static const char
usage_text[] = {
@@ -384,10 +388,12 @@
const char *pos;
char *target, *last;
char **table_pos, **argv;
- const size_t optlen = strlen (opts);
+ size_t optlen;
assert (0 != opts);
+ optlen = strlen (opts);
+
if (0 == optlen) {
/* Alloc a an index array to hold the program name */
argv = (char**)RW_MALLOC (sizeof (char*));
@@ -429,7 +435,7 @@
}
in_token = 1;
switch (*pos) {
- case '\\':
+ case escape_code:
in_escape = 1;
break;
case '"':
Index: output.cpp
===================================================================
--- output.cpp (revision 431602)
+++ output.cpp (working copy)
@@ -232,22 +232,15 @@
check_example (char* const out_name, FILE* fout)
{
struct stat file_info;
- const size_t root_len = strlen (in_root);
- char* const ref_name = (char*)RW_MALLOC (root_len
- + strlen (target_name) + 19);
+ char* ref_name;
FILE* fref; /* reference file (expected output) */
- assert (0 != in_root);
- assert (0 < root_len);
- assert (0 != target_name);
+ assert (0 != out_name);
assert (0 != fout);
/* Try in_root/manual/out/target_name.out */
- memcpy (ref_name, in_root, root_len+1);
- strcat (ref_name, "/manual/out/");
- strcat (ref_name, target_name);
- strcat (ref_name, ".out");
+ ref_name = reference_name ("manual", "out");
if (0 > stat (ref_name, &file_info)) {
if (ENOENT != errno) {
@@ -260,10 +253,8 @@
/* If that doesn't exist, try
in_root/tutorial/out/target_name.out */
- memcpy (ref_name, in_root, root_len+1);
- strcat (ref_name, "/tutorial/out/");
- strcat (ref_name, target_name);
- strcat (ref_name, ".out");
+ free (ref_name);
+ ref_name = reference_name ("tutorial", "out");
if (0 > stat (ref_name, &file_info)) {
if (ENOENT != errno) {
@@ -345,15 +336,12 @@
void
parse_output (const char* target)
{
- const size_t path_len = strlen (target);
- char* const out_name = (char*)RW_MALLOC (path_len + 5);
+ char* out_name;
FILE* data;
assert (0 != target);
+ out_name = output_name (target);
- memcpy (out_name, target, path_len + 1);
- strcat (out_name,".out");
-
data = fopen (out_name, "r");
if (0 == data) {
Index: util.cpp
===================================================================
--- util.cpp (revision 431602)
+++ util.cpp (working copy)
@@ -86,11 +86,13 @@
void*
guarded_malloc (const size_t size, const char* const file, const unsigned line)
{
- void* const alloc = malloc (size);
+ void* alloc;
assert (0 != file);
assert (0 < size);
+ alloc = malloc (size);
+
if (0 == alloc)
terminate (1, "malloc (%lu) at line %u of %s failed: %s\n",
(unsigned long)size, line, file, strerror (errno));
@@ -111,14 +113,70 @@
guarded_realloc (void* source, const size_t size, const char* const file,
const unsigned line)
{
- void* const alloc = realloc (source, size);
+ void* alloc;
assert (0 != file);
assert (0 < size);
+ alloc = realloc (source, size);
+
if ( 0 == alloc )
terminate ( 1, "malloc(%lu) at line %u of %s failed: %s\n",
(unsigned long)size, line, file, strerror (errno));
return alloc;
}
+
+char*
+reference_name (const char* dir, const char* mode)
+{
+ size_t root_len, cmp_len, dir_len, mode_len, net_len;
+ char* ref_name;
+ char* tail;
+
+ assert (0 != in_root);
+ assert (0 != target_name);
+ assert (0 != dir);
+ assert (0 != mode);
+
+ root_len = strlen (in_root);
+ cmp_len = strlen (target_name) - exe_suffix_len;
+ dir_len = strlen (dir);
+ mode_len = strlen (mode);
+ net_len = root_len + cmp_len + dir_len + mode_len * 2 + 5;
+ /* 5 comes from 3 path seperator characters, the suffix seperator
+ character, and the trailing null */
+ tail = ref_name = (char*)RW_MALLOC (net_len);
+
+ memcpy (tail, in_root, root_len);
+ tail += root_len;
+ *tail++ = default_path_sep;
+ memcpy (tail , dir, dir_len);
+ tail += dir_len;
+ *tail++ = default_path_sep;
+ memcpy (tail , mode, mode_len);
+ tail += mode_len;
+ *tail++ = default_path_sep;
+ memcpy (tail , target_name, cmp_len);
+ tail += cmp_len;
+ *tail++ = suffix_sep;
+ memcpy (tail , mode, mode_len);
+ tail += mode_len;
+ *tail = '\0';
+
+ return ref_name;
+}
+
+char*
+output_name (const char* target)
+{
+ const char* suffix = "out";
+ const size_t sfx_len = strlen (suffix);
+ const size_t exe_len = strlen (target) - exe_suffix_len;
+ char* const tmp_name = (char*)RW_MALLOC (exe_len + sfx_len + 2);
+
+ memcpy (tmp_name, target, exe_len);
+ *(tmp_name + exe_len) = suffix_sep;
+ memcpy (tmp_name + exe_len + 1, suffix, sfx_len + 1);
+ return tmp_name;
+}
Index: cmdopt.h
===================================================================
--- cmdopt.h (revision 431602)
+++ cmdopt.h (working copy)
@@ -34,6 +34,10 @@
extern const char* in_root;
extern const char* exe_name;
extern const char* target_name; /**< Alias for current target name. */
+extern const char escape_code; /**< Escape character used in paths. */
+extern const char default_path_sep; /**< Primary path seperator */
+extern const char suffix_sep; /**< File suffix seperator. */
+extern const size_t exe_suffix_len; /**< Length of executable suffix. */
void
show_usage (int status);
Index: runall.cpp
===================================================================
--- runall.cpp (revision 431602)
+++ runall.cpp (working copy)
@@ -69,13 +69,16 @@
static char** const
merge_argv (char* const target, char* const argv [])
{
- const size_t tlen = strlen (target);
- char ** split = split_opt_string (target);
+ size_t tlen;
+ char ** split;
unsigned i, arg_count = 0, spl_count = 0, wld_count = 0;
assert (0 != target);
assert (0 != argv);
+ tlen = strlen (target);
+ split = split_opt_string (target);
+
/* If the split of target only contains a single value, we may have a
bare executable name */
if (!split [1]) {
@@ -211,6 +214,7 @@
const size_t path_len = strlen (target);
char* tmp_name;
+#if 0 /* Disable .o target check as unused */
/* If target is a .o file, check if it exists */
if ('.' == target [path_len-1] && 'o' == target [path_len]) {
if (exists)
@@ -219,6 +223,7 @@
puts (" COMP");
return 0;
}
+#endif
/* If the target exists, it doesn't have valid permissions */
if (exists) {
@@ -330,7 +335,7 @@
assert (0 != path);
for (mark = pos = path; '\0' != *pos; ++pos)
- mark = ('/' == *pos || '\\' == *pos) ? pos + 1 : mark;
+ mark = (default_path_sep == *pos) ? pos + 1 : mark;
return mark;
}
@@ -352,11 +357,15 @@
run_target (char* target, char** argv)
{
struct exec_attrs status;
- char** childargv = merge_argv (target, argv);
+ char** childargv;
assert (0 != target);
assert (0 != argv);
+
+ childargv = merge_argv (target, argv);
+
assert (0 != childargv);
+ assert (0 != childargv [0]);
target_name = rw_basename (childargv [0]);
