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

[Pedro Alves <palves@redhat.com>]

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