Re: [RFC PATCH v5 2/9] Add libiberty/concat styled concat_path function

[Simon Marchi <simark at simark dot ca>]

On 2018-03-12 11:31 AM, Philipp Rudo wrote:
>     This commit adds concat_path function to concatenate an arbitrary number of
>     path elements.  The function automatically adds an directory separator between
>     two elements as needed.
> 
>     gdb/ChangeLog:
> 
> 	* common/common-utils.h (endswith): New function.
> 	* utils.c (_concat_path, approx_path_length): New function.
> 	* utils.h (_concat_path): New export.
> 	(concat_path): New define.
> ---
>  gdb/common/common-utils.h | 11 +++++++++++
>  gdb/utils.c               | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/utils.h               | 16 ++++++++++++++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 5408c35469..8e6eb05c2d 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -109,6 +109,17 @@ startswith (const char *string, const char *pattern)
>    return strncmp (string, pattern, strlen (pattern)) == 0;
>  }
>  
> +/* Return non-zero if the end of STRING matches PATTERN, zero
> +   otherwise.  */
> +
> +static inline int
> +endswith (const char *string, const char *pattern)
> +{
> +  return (strlen (string) > strlen (pattern)

Should this be >=, so that endswith ("Hello", "Hello") returns true?

> +	  && strncmp (string + strlen (string) - strlen (pattern), pattern,
> +		      strlen (pattern)) == 0);
> +}

I believe you can use strcmp, which would avoid the last call to strlen.


> +
>  ULONGEST strtoulst (const char *num, const char **trailer, int base);
>  
>  /* Skip leading whitespace characters in INP, returning an updated
> diff --git a/gdb/utils.c b/gdb/utils.c
> index d4f1398d14..4ce3909fca 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3072,6 +3072,52 @@ substitute_path_component (std::string &str, const std::string &from,
>      }
>  }
>  
> +/* Approximate length of final path.  Helper for concat_path.  */
> +
> +static inline unsigned long

Make the return type size_t too?

> +approx_path_length (const std::initializer_list<const std::string> args,
>
> +		    const std::string &dir_separator)
> +{
> +  size_t length = 0;
> +
> +  for (const std::string &arg: args)
> +      length += arg.length () + dir_separator.length ();

One extra indentation level.

> +
> +  return length;
> +}
> +
> +/* See utils.h.  */
> +
> +std::string
> +_concat_path (const std::initializer_list<const std::string> args,
> +	      const std::string &dir_separator)

dit_separator should probably be a const char *, for the same reason
as mentioned in the previous patch.

> +{
> +  std::string ret;
> +  ret.reserve (approx_path_length (args, dir_separator));
> +
> +  for (const std::string &arg : args)
> +    {
> +      if (arg.empty ())
> +	continue;
> +
> +      if (startswith (arg.c_str (), dir_separator.c_str ())
> +	  && endswith (ret.c_str (), dir_separator.c_str ()))
> +	ret.erase (ret.length () - dir_separator.length (),
> +		   dir_separator.length ());
> +
> +      else if (!ret.empty ()
> +	       && !startswith (arg.c_str (), dir_separator.c_str ())
> +	       && !endswith (ret.c_str (), dir_separator.c_str ())
> +	       && ret != TARGET_SYSROOT_PREFIX)
> +	ret += dir_separator;
> +
> +      ret += arg;
> +    }
> +
> +  ret.shrink_to_fit ();
> +  return ret;
> +}
> +
>  #ifdef HAVE_WAITPID
>  
>  #ifdef SIGALRM
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 7e6a39ee82..aec9c6194d 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -307,6 +307,22 @@ extern void substitute_path_component (std::string &str,
>  				       const std::string &from,
>  				       const std::string &to);
>  
> +/* Concatenate an arbitrary number of path elements.  Adds and removes
> +   directory separators as needed.

Maybe mention the exception for TARGET_SYSROOT_PREFIX?

> +
> +   concat_path (/first, second)		=> /first/second
> +   concat_path (first, second)		=> first/second
> +   concat_path (first/, second)		=> first/second
> +   concat_path (first, /second)		=> first/second
> +   concat_path (first/, /second)	=> first/second
> +   concat_path (target:, second)	=> target:second

Should these examples include quotes, eg:

  concat_path ("/first", "second")		=> /first/second

?

Also, those examples would be nice to convert to a selftest/unittest in the
newly created unittests/utils-selftests.c.  :)

> +   */
> +
> +extern std::string _concat_path (const std::initializer_list<const std::string> args,
> +				 const std::string &dir_separator);
> +
> +#define concat_path(...) _concat_path ({__VA_ARGS__}, SLASH_STRING)
> +
>  std::string ldirname (const char *filename);
>  
>  extern int count_path_elements (const char *path);
> 

Thanks,

Simon