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

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

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