From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id k/4kIdmL+1+6AwAAWB0awg (envelope-from ) for ; Sun, 10 Jan 2021 18:20:57 -0500 Received: by simark.ca (Postfix, from userid 112) id 77F051EE85; Sun, 10 Jan 2021 18:20:57 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id EC1651EE1B for ; Sun, 10 Jan 2021 18:20:55 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B7274385803C; Sun, 10 Jan 2021 23:20:54 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B7274385803C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1610320854; bh=j7drma/hR8/D999EDO0baFXeKD+ZR0b3GvlWtfjtyFM=; h=To:Subject:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=WG0fqA6S7FVjFBpzxtfCsW0lyyql/ISITlE0/dwdN8LJSEinTWjChsJYTFTWjtGf1 AAQGkuu+JCuq47RFWFa0q8r6D8ecg7FXxY6qIU+qu6xC7r/afwSNuCLI/mHq8X0BBg ho6U2dAtrt9aWx40bO6gFxyVWVflLT2yAPI0gHy8= Received: from mail.sergiodj.net (mail.sergiodj.net [IPv6:2607:5300:60:3666::3]) by sourceware.org (Postfix) with ESMTPS id DCCE8385803C for ; Sun, 10 Jan 2021 23:20:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DCCE8385803C Received: from localhost (bras-base-toroon1016w-grc-31-76-64-52-221.dsl.bell.ca [76.64.52.221]) by mail.sergiodj.net (Postfix) with ESMTPSA id 385E4A058C; Sun, 10 Jan 2021 18:20:51 -0500 (EST) To: Lancelot SIX via Gdb-patches Subject: Re: [PATCH v2] Improve gdb_tilde_expand logic References: <20210108001337.29164-1-lsix@lancelotsix.com> <20210109182906.24570-1-lsix@lancelotsix.com> X-URL: http://blog.sergiodj.net Date: Sun, 10 Jan 2021 18:20:51 -0500 In-Reply-To: <20210109182906.24570-1-lsix@lancelotsix.com> (Lancelot SIX via Gdb-patches's message of "Sat, 9 Jan 2021 18:29:06 +0000") Message-ID: <87y2h0peek.fsf@paluero> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergio Durigan Junior via Gdb-patches Reply-To: Sergio Durigan Junior Cc: Lancelot SIX Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On Saturday, January 09 2021, Lancelot SIX via Gdb-patches wrote: > Before this patch, gdb_tilde_expand would use glob(3) in order to expand > tilde at the begining of a path. This implementation has limitation when > expanding a tilde leading path to a non existing file since glob fails to > expand. > > This patch proposes to use glob only to expand the tilde component of the > path and leaves the rest of the path unchanged. > > This patch is a followup to the following discution: > https://sourceware.org/pipermail/gdb-patches/2021-January/174776.html > > Before the patch: > > gdb_tilde_expand("~") -> "/home/lsix" > gdb_tilde_expand("~/a/c/b") -> error() is called > > After the patch: > > gdb_tilde_expand("~") -> "/home/lsix" > gdb_tilde_expand("~/a/c/b") -> "/home/lsix/a/c/b" > > Tested on x84_64 linux. > > New since V1: > * Unittests added thanks to inputs and skeleton provided by > Simon Marchi. > * My copyright assignment has been signed! > > gdb/ChangeLog: > > * Makefile.in (SELFTESTS_SRCS): Add > unittests/gdb_tilde_expand-selftests.c. > * unittests/gdb_tilde_expand-selftests.c: New file. > > gdbsupport/ChangeLog: > > * gdb_tilde_expand.cc (gdb_tilde_expand): Improve > implementation. > (gdb_tilde_expand_up): Delegate logic to gdb_tilde_expand. > * gdb_tilde_expand.h (gdb_tilde_expand): Update description. > --- > gdb/Makefile.in | 1 + > gdb/unittests/gdb_tilde_expand-selftests.c | 62 ++++++++++++++++++++++ > gdbsupport/gdb_tilde_expand.cc | 45 +++++++++++----- > gdbsupport/gdb_tilde_expand.h | 3 +- > 4 files changed, 97 insertions(+), 14 deletions(-) > create mode 100644 gdb/unittests/gdb_tilde_expand-selftests.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 9267bea7be..c8979fe276 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -446,6 +446,7 @@ SELFTESTS_SRCS = \ > unittests/filtered_iterator-selftests.c \ > unittests/format_pieces-selftests.c \ > unittests/function-view-selftests.c \ > + unittests/gdb_tilde_expand-selftests.c \ > unittests/gmp-utils-selftests.c \ > unittests/lookup_name_info-selftests.c \ > unittests/memory-map-selftests.c \ > diff --git a/gdb/unittests/gdb_tilde_expand-selftests.c b/gdb/unittests/gdb_tilde_expand-selftests.c > new file mode 100644 > index 0000000000..464e63b2e4 > --- /dev/null > +++ b/gdb/unittests/gdb_tilde_expand-selftests.c > @@ -0,0 +1,62 @@ > +/* Self tests for gdb_tilde_expand > + > + Copyright (C) 2021 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "gdbsupport/common-defs.h" > +#include "filenames.h" > +#include "gdbsupport/selftest.h" > + > +#include "gdbsupport/gdb_tilde_expand.h" > + > +namespace selftests { > +namespace gdb_tilde_expand_tests { > + > +static void > +do_test () > +{ > + /* Home directory of the user running the test */ Period at the end of the sentence. > + const std::string home = gdb_tilde_expand ("~"); I think it doesn't make much sense to rely on gdb_tilde_expand to test gdb_tilde_expand. I think it's better to use $HOME, and just don't run the test if it's not defined. Or (thinking aloud here) you could call glob(3) directly here. > + // The result should be a absolute path > + SELF_CHECK (IS_ABSOLUTE_PATH (home)); IIRC // comments are not used on GDB. > + > + /* When given a path that beging by a tilde and refers to a file that s/beging/begins/ > + does not exist, gdb_tilde expand must still be able to do the tilde > + expansion. */ Two spaces after period (here and everywhere else). > + SELF_CHECK > + (gdb_tilde_expand ("~/non/existent/directory") == > + home + "/non/existent/directory"); > + > + /* gdb_tilde_expand only expands tilde and does not try to do any other > + substitution. */ > + SELF_CHECK (gdb_tilde_expand ("~/*/a.out") == home + "/*/a.out"); > + > + /* gdb_tilde_expand does no modification to a non tilde leading path. */ > + SELF_CHECK (gdb_tilde_expand ("/some/abs/path") == "/some/abs/path"); > + SELF_CHECK (gdb_tilde_expand("relative/path") == "relative/path"); Space before open parenthesis. > +} > + > +} /* namespace gdb_tilde_expand_tests */ I was going to say that I think this function should include tests for the "~user/path" scenario, but I don't know offhand how you could do that. The challenge here would be knowing the user. Maybe you can write something that relies on $USER being defined, not sure. > +} /* namespace selftests */ > + > +void _initialize_gdb_tilde_expand_selftests (); > +void > +_initialize_gdb_tilde_expand_selftests () > +{ > + selftests::register_test > + ("gdb_tilde_expand", selftests::gdb_tilde_expand_tests::do_test); > +} > diff --git a/gdbsupport/gdb_tilde_expand.cc b/gdbsupport/gdb_tilde_expand.cc > index b31fc48446..d060e26f50 100644 > --- a/gdbsupport/gdb_tilde_expand.cc > +++ b/gdbsupport/gdb_tilde_expand.cc > @@ -17,7 +17,9 @@ > You should have received a copy of the GNU General Public License > along with this program. If not, see . */ > > +#include "common-defs.h" should always be the first include in a file. > #include "common-defs.h" > +#include "filenames.h" > #include "gdb_tilde_expand.h" > #include > > @@ -71,14 +73,37 @@ private: > std::string > gdb_tilde_expand (const char *dir) > { > - gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL); > + if (dir[0] == '~') > + { > + /* This function uses glob in order to expand the ~. However, this > + function will fail to expand if the actual dir we are looking > + for does not exist. Given "~/does/not/exist", glob will fail. The two lines above are indented differently (the first one is using only spaces, the second one is using TAB). > > - gdb_assert (glob.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. */ > - std::string expanded_dir = glob.pathv ()[0]; > + In order to avoid such limitation, we only use > + glob to expand "~" and keep "/does/not/exist" unchanged. > > - return expanded_dir; > + Similarly, to expand ~gdb/might/not/exist, we only expand > + "~gdb" using glob and leave "/might/not/exist" unchanged. */ > + > + const std::string d (dir); > + > + // Look for the first dir separator (if any) > + const auto first_sep = The equal sign should be at the beginning of the next line. However, I don't think you really need to break the line here. > + std::find_if (d.cbegin (), d.cend(), > + [](const auto c) { return IS_DIR_SEPARATOR (c); }); Matter of personal taste, but I'm really not a fan of the "use auto everywhere" idiom here. > + > + // split d according to the first separator > + const std::string to_expand (d.cbegin (), first_sep); > + const std::string remainder (first_sep, d.cend ()); > + > + // Expand the first element > + const gdb_glob glob (to_expand.c_str (), GLOB_TILDE_CHECK, nullptr); > + gdb_assert (glob.pathc () == 1); You can move the comment you deleted earlier to here: /* "glob" may return more than one match to the path provided by the user, but we are only interested in the first match. */ > + > + return std::string (glob.pathv ()[0]) + remainder; > + } > + else > + return std::string (dir); You can get rid of the new indentation level you're adding by inverting the check at the beginning of the function: if (dir[0] != '~') return std::string (dir); ... > } > > /* See gdbsupport/gdb_tilde_expand.h. */ > @@ -86,10 +111,6 @@ gdb_tilde_expand (const char *dir) > gdb::unique_xmalloc_ptr > gdb_tilde_expand_up (const char *dir) > { > - gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL); > - > - gdb_assert (glob.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. */ > - return make_unique_xstrdup (glob.pathv ()[0]); > + auto const expanded = gdb_tilde_expand (dir); Same comment here about "auto everywhere". > + return make_unique_xstrdup (expanded.c_str ()); > } > diff --git a/gdbsupport/gdb_tilde_expand.h b/gdbsupport/gdb_tilde_expand.h > index e2d85cadad..a61f246329 100644 > --- a/gdbsupport/gdb_tilde_expand.h > +++ b/gdbsupport/gdb_tilde_expand.h > @@ -20,8 +20,7 @@ > #ifndef COMMON_GDB_TILDE_EXPAND_H > #define COMMON_GDB_TILDE_EXPAND_H > > -/* Perform path expansion (i.e., tilde expansion) on DIR, and return > - the full path. */ > +/* Perform tilde expansion on DIR, and return the full path. */ > extern std::string gdb_tilde_expand (const char *dir); > > /* Same as GDB_TILDE_EXPAND, but return the full path as a > -- > > 2.29.2 > > -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible https://sergiodj.net/