From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29336 invoked by alias); 13 Sep 2017 16:07:07 -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 29326 invoked by uid 89); 13 Sep 2017 16:07:07 -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=unsatisfied 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, 13 Sep 2017 16:07:06 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1A20BC04B92E for ; Wed, 13 Sep 2017 16:07:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1A20BC04B92E Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com 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 88DB360600; Wed, 13 Sep 2017 16:07:03 +0000 (UTC) Subject: Re: [PATCH 3/4] Introduce gdb_chdir To: Sergio Durigan Junior , GDB Patches References: <20170912042325.14927-1-sergiodj@redhat.com> <20170912042325.14927-4-sergiodj@redhat.com> From: Pedro Alves Message-ID: <6f978544-e1d4-b921-2e10-6be7f0e6b563@redhat.com> Date: Wed, 13 Sep 2017 16:07: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: <20170912042325.14927-4-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-09/txt/msg00362.txt.bz2 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. > +/* 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. > +{ > + 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]; > + 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... > + > + 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. > + > + return expanded_dir; > +} > +