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

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

Hi Pedro,

sorry for the late answer but I was sick the last 10 days and just returned
yesterday...

On Wed, 17 May 2017 17:06:13 +0100
Pedro Alves <palves@redhat.com> wrote:

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

The patch can be split from my series without problem.  I already pulled it
right to the beginning of the series, as it is independent of all other changes
in the set.

There are quite some places you can simplify by using the function.  Just grep
for SLASH_STRING to find most of them.  I also had a patch once where I
eliminated all use of SLASH_STRING using the concat_path.  Unfortunately it had
some problems with the mixture of C and C++ strings causing memory leaks and
read after free's.  I never had the time to clean that up and thus didn't send
it to the mailing list ...
 
> 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.

True, this can be optimized.

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

Also true, let me see if I can hack something today.  Currently I don't like to
do what I should do and there are many meeting today anyway.  So this looks like
a perfect task for today ;)

Philipp
 
> Thanks,
> Pedro Alves
>