Re: [PATCH 1/5] Introduce build_debug_file_name

[Pedro Alves <palves@redhat.com>]

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