From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 8DB653858D3C for ; Wed, 20 Apr 2022 00:23:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8DB653858D3C Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 23K0MdwJ017357 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 19 Apr 2022 20:22:44 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 23K0MdwJ017357 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 04ABF1E150; Tue, 19 Apr 2022 20:22:37 -0400 (EDT) Message-ID: <7c4c5b28-843a-f434-35e2-d42f51f18321@polymtl.ca> Date: Tue, 19 Apr 2022 20:22:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH 5/5] gdbsupport: add path_join function Content-Language: en-US To: Lancelot SIX Cc: gdb-patches@sourceware.org, Simon Marchi References: <20220414200137.3479373-1-simon.marchi@polymtl.ca> <20220414200137.3479373-5-simon.marchi@polymtl.ca> <20220415143827.t2nlcfhmh2pondev@ubuntu.lan> <20220418231116.qhpo34hnaxpjwzcs@ubuntu.lan> From: Simon Marchi In-Reply-To: <20220418231116.qhpo34hnaxpjwzcs@ubuntu.lan> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 20 Apr 2022 00:22:39 +0000 X-Spam-Status: No, score=-3034.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 20 Apr 2022 00:23:50 -0000 >>> Is this different semantics made on purpose? >> >> I indeed noticed how os.path.join handled ("/foo", "/bar"). >> >> I went with the behavior in my patch mostly because there are cases in >> GDB where we really need to append absolute paths to another path. For >> example, looking up files in sysroots. If the sysroot is set to >> "/the/sysroot/" (with or without the trailing slash) and we want to load >> the target library /usr/lib/libfoo.so, it would be nice if >> >> path_join (sysroot_path, lib_path) >> >> gave >> >> /the/sysroot/usr/lib/libfoo.so >> >> without a double-slash. Same when we are looking up files in the >> debug-file-directory (/usr/lib/debug). >> >> When changing those implementations that currently use concat(...) or >> string_printf("%s/%s"), we would need to be careful: if the right hand >> side is an absolute path, then we would have to skip the leading >> directory separator before passing it to path_join, to get the desired >> result. And then we're back at having some special path handling >> details everywhere. > > Fair enough. This is a good enough reason to have a special semantics > (we could have a path_join and a sysroot_path_join, but not sure it is > worth it having 2 functions). Well, I've been thinking about it, and perhaps that appending an absolute path to another path (either absolute or non-absolute), sysroot-style, is indeed a distinct use case that a regular path_join is not meant to solve. The code that Pedro pointed to, in solib_find_1, convinced me. It tries different things when dealing with a Windows paths that contains a drive letter. And compatibility-wise, being a drop-in replacement for std::filesystem::operator/ is interesting. > >> If you can make it work using gdb::string_view or const char *, I think >> it would be ok. I'll give it a try. But otherwise, I am personally >> fine with the sentinel nullptr, given that the compiler gives you a >> warning if you forget it. >> > > Something like this avoids copying some std::string around (which my > initial implementation did), and can work with a string view (would be > the same architecture with const char*). The compiler should optimize > almost all the function calls away easily enough. I saw that Tom gave > another approach which would work fine as well. > > namespace impl { > > template > inline void > path_join_1 (std::string &l, gdb::string_view r, Args... comps) > { > path_join_1 (l, r); > path_join_1 (l, ...comps); > } > > template<> > inline void > path_join_1 (std::string &l, gdb::string_view r) > { > /* The real logic here */ > } > > } /* namespace impl */ > > template > inline std::string > path_join (Args... comps) > { > std::string res; > impl::path_join_1 (res, comps...); > return res; > } Thanks for the suggestion, I ended up using Tom's proposal. Simon