From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89301 invoked by alias); 17 May 2017 16:06:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 88933 invoked by uid 89); 17 May 2017 16:06:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=scenes, justified X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 May 2017 16:06:12 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AF4FA80C0D; Wed, 17 May 2017 16:06:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AF4FA80C0D 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@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com AF4FA80C0D Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id D435817503; Wed, 17 May 2017 16:06:13 +0000 (UTC) Subject: Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml To: Philipp Rudo , Yao Qi References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <1494518105-15412-3-git-send-email-yao.qi@linaro.org> <20170516140027.29636db3@ThinkPad> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <4c6f2f5a-e2c2-df09-0440-0f9431e7594a@redhat.com> Date: Wed, 17 May 2017 16:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170516140027.29636db3@ThinkPad> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00402.txt.bz2 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 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