Re: [PATCH 1/5] Introduce build_debug_file_name

Pedro Alves
2015-07-01T11:05:46Z

On 06/16/2015 10:42 AM, Gary Benson wrote:
> This commit introduces a new function build_debug_file_name which
> concatenates a series of filename components into a filename.
> find_separate_debug_file is updated to use build_debug_file_name.
> A later commit in this series will extend build_debug_file_name
> to correctly handle "target:" prefixes, so it is convenient to
> have filename building pulled out into one function. For now the
> only functional change here is that the original code sometimes
> generated filenames with repeated directory separators while the
> new code does not.

I'd drop the "debug" from the function's name. Sounds like a
candidate for reuse elsewhere to me.

>
> gdb/ChangeLog:
>
> * gdb/symfile.c (build_debug_file_name): New function.
> (find_separate_debug_file): Use the above to build filenames.
> ---
> gdb/ChangeLog | 5 ++
> gdb/symfile.c | 117 +++++++++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 90 insertions(+), 32 deletions(-)
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 0c35ffa..799133a 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1431,6 +1431,79 @@ separate_debug_file_exists (const char *name, unsigned long crc,
> return 1;
> }
>
> +/* Build the filename of a separate debug file from an arbitrary
> + number of components. Returns an xmalloc'd string that must
> + be be freed by the caller. The final argument of this function
> + must be NULL to mark the end the argument list. */

double "be be".

> +
> +static char *
> +build_debug_file_name (const char *first, ...)
> +{
> + va_list ap;
> + const char *arg, *last;
> + VEC (char_ptr) *args = NULL;
> + struct cleanup *back_to = make_cleanup_free_char_ptr_vec (args);
> + int bufsiz = 0;
> + char *buf, *tmp;
> + int i;
> +
> + va_start (ap, first);
> + for (arg = first; arg; arg = va_arg (ap, const char *))

arg != NULL in predicate.

> + last = arg;
> + va_end (ap);
> +
> + va_start (ap, first);
> + for (arg = first; arg; arg = va_arg (ap, const char *))

likewise.

> + {
> + if (arg == last)
> + tmp = xstrdup (arg);
> + else
> + {
> + int len;
> +
> + /* Strip leading separators from subdirectories. */
> + if (arg != first)
> + {
> + while (*arg != '\0' && IS_DIR_SEPARATOR (*arg))
> + arg++;
> + }
> +
> + /* Strip trailing separators. */
> + len = strlen (arg);
> +
> + while (len > 0 && IS_DIR_SEPARATOR (arg[len - 1]))
> + len--;
> +
> + if (len > 0)
> + {
> + tmp = xmalloc (len + strlen (SLASH_STRING) + 1);
> + memcpy (tmp, arg, len);
> + strcpy (tmp + len, SLASH_STRING);
> + }
> + else
> + tmp = NULL;
> + }
> +
> + if (tmp != NULL)
> + {
> + VEC_safe_push (char_ptr, args, tmp);
> + bufsiz += strlen (tmp);

Why build the temporary VEC instead of just incrementally
building the final buf?

I think you could simplify this much if you did that,
and plus use reconcat.

> + }
> + }
> + va_end (ap);
> +
> + bufsiz += 1; /* Terminator. */
> +
> + buf = xmalloc (bufsiz);
> + buf[0] = '\0';
> + for (i = 0; VEC_iterate (char_ptr, args, i, tmp); i++)
> + strcat (buf, tmp);
> + gdb_assert (bufsiz == strlen (buf) + 1);

Thanks,
Pedro Alves