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

  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 (startswith): Convert to C++.
	(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 | 16 +++++++++++++---
 gdb/utils.c               | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/utils.h               | 17 +++++++++++++++++
 3 files changed, 76 insertions(+), 3 deletions(-)
  

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
  

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
>
  

On 01/12/2017 01:33 PM, Philipp Rudo wrote:
> 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 if we used std::string throughout, we should still be careful
with unnecessary string copying.  "std::string" vs "const string &"
in function parameters (use the former only when the function already
needs to work with a copy).  

Similarly, please don't write:

+  for (std::string arg: args)
+    {

Please write instead:

 for (const std::string &arg : args)

Or 

 for (const auto &arg : args)

"for (std::string arg: args)" creates/destroys
one deep string copy on each iteration.

I hope it's obvious that I'm all for C++ conversion, but ...

> Even when it costs performance.

... not at any cost.  startswith _is_ used in performance
critical paths.

BTW, I've been thinking that we may want to add our version
of C++17 std::string_view to avoid these kinds of problems.

Thanks,
Pedro Alves
  

On Thu, 12 Jan 2017 13:48:03 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 01/12/2017 01:33 PM, Philipp Rudo wrote:
> > 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 if we used std::string throughout, we should still be careful
> with unnecessary string copying.  "std::string" vs "const string &"
> in function parameters (use the former only when the function already
> needs to work with a copy).  

You are right here.

> Similarly, please don't write:
> 
> +  for (std::string arg: args)
> +    {
> 
> Please write instead:
> 
>  for (const std::string &arg : args)
> 
> Or 
> 
>  for (const auto &arg : args)
> 
> "for (std::string arg: args)" creates/destroys
> one deep string copy on each iteration.

Thanks for the tip. I will check my patches again.
 
> I hope it's obvious that I'm all for C++ conversion, but ...

its quite obvious that you are pro C++. My own feelings are quite
mixed. I must amid that the standard library is quite handy, at least
when it works. Debugging it is quite a pain. But the syntax sometimes
is unreadable, especially when you use those teplates containing
templates using types is different namespaces.
Furthermore i don't think that simply porting GDB to C++ solves its
major problem that there are no clear structures in code. Although it
could help if you not only port it one to one but restructure the code
while you work at it. But that would requires quite some work...

> > Even when it costs performance.  
> 
> ... not at any cost.  startswith _is_ used in performance
> critical paths.
>
> BTW, I've been thinking that we may want to add our version
> of C++17 std::string_view to avoid these kinds of problems.

As much as i know about C++ this sounds like a good idea.

Thanks
Philipp
  

On 01/12/2017 03:08 PM, Philipp Rudo wrote:
> its quite obvious that you are pro C++. My own feelings are quite
> mixed. I must amid that the standard library is quite handy, at least
> when it works. Debugging it is quite a pain.

There's definitely lots of scope for making C++ debugging less painful.
I think 2017 will see good advancements here.  For example, with the
palves/cp-linespec branch on my github, setting breakpoints in C++ methods
is soooooo much easier, mainly because I've made linespec tab completion
Just Work.  The whole "compile" feature for C++ (making use of g++ for
the parsing, via the libcc1 plugin) should be making it upstream this year.
Etc.

Keep in mind that a significant (if not the largest) chunk of
our users is using GDB to debug their C++ code too.  So in a sense,
any pain we now may feel, a good chunk of our users have been feeling
for a long while.  We just hadn't been dogfooding.

> But the syntax sometimes
> is unreadable, especially when you use those teplates containing
> templates using types is different namespaces.

OTOH, stepping through C code that emulates templates using
C #defines OTOH is just plain impossible, since all of it
is compiled down to a single source line...  So in that sense,
I think debugging C++ is better than C here.

It also helps if you're using a distro that installs pretty printers
for the standard library correctly.  Fedora does.  Ubuntu didn't use
to, but I don't know the current state.

Thanks,
Pedro Alves