Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml

[Pedro Alves <palves@redhat.com>]

On 05/16/2017 01:00 PM, Philipp Rudo wrote:

>> +  /* Look for the features directory.  If the directory of __FILE__ can't
>> +     be found, __FILE__ is a file name with relative path.  Guess that
>> +     GDB is executed in testsuite directory like ../gdb, because I don't
>> +     expect that GDB is invoked somewhere else and run self tests.  */
>> +  if (stat (feature_dir.data (), &st) < 0)
>> +    {
>> +      feature_dir.insert (0, SLASH_STRING);
>> +      feature_dir.insert (0, "..");
> 
> This would be a perfect spot for concat_path (patch 2 from my lk series
> https://sourceware.org/ml/gdb-patches/2017-03/msg00272.html).
> Then it would read
> 
> feature_dir = concat_path ("..", feature_dir);
> 
> I actually want to bring some of my patches upstream (mostly the
> s390-linux-tdep split up patch) to reduce maintenance cost of my
> series.  This would be a good time for this patch, too.

Could that patch be split out of the series, and justified on its
own grounds?  Is there some representative place in the codebase
that you could cleanup a bit using the new API, just to show it
working nicely?  Also, a unit test would be nice.

Also, and most importantly, seems to me that patch still has
a lot inefficiency related to std::string copying, e.g.:

 +static inline unsigned long
 +approx_path_length (std::initializer_list<std::string> args,
 +		   std::string dir_separator)

This is passing in the strings by value / copy.  That looks like
an aweful lot of malloc/free/strdup behind the scenes.

I still think that if we're adding some utility for building
paths from dir components, that it'd be preferred to model
the API on (a small subset of) std::experimental::filesystem::path,
since in a few years that's the API that everyone learning C++ will
be learning.

Thanks,
Pedro Alves