Re: [RFC 2/7] Add libiberty/concat styled concat_path function

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

Hi Pedro,

i see your point. 

My goal here was to get rid of any C-string. While making this patch i
also wanted to use it to get rid of all those

concat (path, need_dirsep ? SLASH_STRING : "", NULL)

or

strcat (path, "/")
strcat (path, file)

constructs. I gave up when it repeatedly caused memory leaks and use
after free errors because of the mixture of C and C++ strings. Fixing
them made the code less readable than before. Thus you should only use
one kind of string through out GDB, either char * or std::string. And
as GDB decided to move to C++ for me std::string is the way you should
go. Even when it costs performance.

Just my 0.02$
Philipp


On Thu, 12 Jan 2017 12:00:22 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 01/12/2017 11:32 AM, Philipp Rudo wrote:
> >  static inline int
> > -startswith (const char *string, const char *pattern)
> > +startswith (std::string str, std::string pattern)  
> 
> NAK.
> 
> This is passing by copy, so will force unnecessary deep
> string copies all over the place.
> 
> >  {
> > -  return strncmp (string, pattern, strlen (pattern)) == 0;
> > +  return (str.find (pattern) == 0);
> > +}
> > +  
> 
> I.e., before, this caused 0 copies:
> 
>   startswith ("foo, "f");
> 
> After, you force a deep string copy for "foo", and another
> for "f".  It's as if you wrote:
> 
>   startswith (xstrdup ("foo), xstrdup ("f"));
> 
> 
> Also, this function is a static inline in a header so that
> the compiler can see when "pattern" is a string literal, and
> thus strlen can be optimized to a plain 'sizeof (pattern) - 1',
> which is very frequent.
> 
> If you want to add overloads that can take "const std::string &"
> for convenience, to avoid str.c_str(), that's maybe fine,
> but you'd have to add all the combinations of
> 'const char *' x 'const std::string &' in the parameters, I
> suppose.
> 
> Thanks,
> Pedro Alves
>