Re: [RFC PATCH v5 1/9] Convert substitute_path_component to C++

[Simon Marchi <simark at simark dot ca>]

On 2018-03-15 10:15 PM, Simon Marchi wrote:
>> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
>> index 70bddbc862..a7f9635252 100644
>> --- a/gdb/auto-load.c
>> +++ b/gdb/auto-load.c
>> @@ -175,21 +175,18 @@ std::vector<gdb::unique_xmalloc_ptr<char>> auto_load_safe_path_vec;
>>     substitute_path_component.  */
>>  
>>  static std::vector<gdb::unique_xmalloc_ptr<char>>
>> -auto_load_expand_dir_vars (const char *string)
>> +auto_load_expand_dir_vars (const std::string &string)
> 
> All the usages of auto_load_expand_dir_vars pass in a char pointer.  This means that
> a temporary std::string is created for the duration of the call (one copy) and another
> one is done lower.  I would suggest either to leave the parameter as const char * to
> avoid that copy.

I stumbled on the discussion you had with Pedro on the v1 patchset:

  https://sourceware.org/ml/gdb-patches/2017-01/msg00210.html

so I thought it would be good to expand a little bit.  A quote from that thread:

> 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.

I don't think we should be so dogmatic.  There are times where a "const char *" is
appropriate, others where std::string is appropriate.  A const std::string& is not
appropriate for a function that is potentially called with string literals, since it
will always require a copy.  As mentioned in that thread, backporting std::string_view
would be the way to go.  But until then, I think we should keep using const char *
in those cases.

Simon