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

Pedro Alves
2017-01-12T13:48:31Z

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