From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85354 invoked by alias); 14 Sep 2017 15:14:46 -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 85338 invoked by uid 89); 14 Sep 2017 15:14:45 -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= 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; Thu, 14 Sep 2017 15:14:44 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4FC937CDE0 for ; Thu, 14 Sep 2017 15:14:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4FC937CDE0 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27CE262929; Thu, 14 Sep 2017 15:14:43 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches Subject: Re: [PATCH 3/4] Introduce gdb_chdir References: <20170912042325.14927-1-sergiodj@redhat.com> <20170912042325.14927-4-sergiodj@redhat.com> <6f978544-e1d4-b921-2e10-6be7f0e6b563@redhat.com> Date: Thu, 14 Sep 2017 15:14:00 -0000 In-Reply-To: <6f978544-e1d4-b921-2e10-6be7f0e6b563@redhat.com> (Pedro Alves's message of "Wed, 13 Sep 2017 17:07:02 +0100") Message-ID: <87vakluxb1.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00379.txt.bz2 On Wednesday, September 13 2017, Pedro Alves wrote: > On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote: > >> +#include "common-defs.h" >> +#include >> +#include "gdb_chdir.h" > > As per the guidelines, the module's own header should be > included first thing right after "common-defs.h". That > allows exposing unsatisfied dependencies (missing includes) > in the header, if any is missing. Done. >> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return >> + the full path. */ >> + >> +static std::string >> +expand_path (const char *dir) > > Since this is particularly about tilde expansion, > and a replacement for "tilde_expand", did you consider calling > it gdb_tilde_expand and using it throughout? If this were an > extern function, I'd press for having "tilde" in its name, > to make the call sites a bit more obvious. Sure, no problem in renaming it. Just to clarify: when you mean "use it throughout", are saying that this should be used to replace readline's "tilde_expand" elsewhere on GDB? >> +{ >> + glob_t g; >> + std::string expanded_dir; > > Move declaration further below to the initialization line, > to avoid pointlessly default-constructing the string: > > std::string expanded_dir = g.gl_pathv[0]; Done. >> + int err = glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, > NULL, &g); >> + >> + if (err != 0) >> + { >> + if (err == GLOB_NOMATCH) >> + error (_("No such directory '%s'. Failure to set cwd."), dir); > > The "Failure to set cwd" string doesn't seem like an error that > should be in "expand_path"? OK, not really an issue since this > is a single-use static function... You're right. I'll make sure to display this error on "gdb_chdir" instead. >> + >> + error (_("Could not process directory '%s'."), dir); >> + } >> + >> + gdb_assert (g.gl_pathc > 0); >> + /* "glob" may return more than one match to the path provided by the >> + user, but we are only interested in the first match. */ >> + expanded_dir = g.gl_pathv[0]; >> + globfree (&g); > > Pedantically, this isn't strictly exception safe, since > either the gdb_assert or the expanded_dir initialization > (a string dup which can fail with out of memory) could throw. > > I think I'd write a thin RAII wrapper around glob that'd have a > single "glob_t m_g;" field, that'd call glob in the ctor, and > globfree in the dtor. OK, I can do that, no problem. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/