This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml
- From: Pedro Alves <palves at redhat dot com>
- To: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>, Yao Qi <qiyaoltc at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 17 May 2017 17:06:13 +0100
- Subject: Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com AF4FA80C0D
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AF4FA80C0D
- References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <1494518105-15412-3-git-send-email-yao.qi@linaro.org> <20170516140027.29636db3@ThinkPad>
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