From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id A38523857C4E for ; Fri, 15 Apr 2022 14:38:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A38523857C4E Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 26B34818C2; Fri, 15 Apr 2022 14:38:32 +0000 (UTC) Date: Fri, 15 Apr 2022 14:38:27 +0000 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org, Simon Marchi Subject: Re: [PATCH 5/5] gdbsupport: add path_join function Message-ID: <20220415143827.t2nlcfhmh2pondev@ubuntu.lan> References: <20220414200137.3479373-1-simon.marchi@polymtl.ca> <20220414200137.3479373-5-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220414200137.3479373-5-simon.marchi@polymtl.ca> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Fri, 15 Apr 2022 14:38:32 +0000 (UTC) X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, 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: Fri, 15 Apr 2022 14:38:36 -0000 Hi, > +/* Join COMPONENT and all subsequent null-terminated string arguments into a > + single path. > + > + The last argument must be nullptr to denote the end of the argument list > + > + Empty arguments or arguments consisting only of directory separators are > + ignored. */ > + > +extern std::string path_join (const char *component, ...) > + ATTRIBUTE_SENTINEL; The last part of the interface seems odd to me. Mostly because it is quite different to the os.path.join API from python. Most notable, in python `os.path.join (a, b)` will yield b if b is an absolute path: >>> os.path.join("/home/foo", "/") '/' >>> os.path.join("/foo", "/bar") '/bar' One rational for this behavior would be to try to mimic using cd on each argument in order: def strange_join(*args): cwd = os.getcwd() try: for comp in args: os.chdir(comp) return os.getcwd() finally: os.chdir(cwd) Of course this implementation would be wrong for multiple reasons: - it must work for dirs which do not exist - the ".." remain ".." - many other... This behaviour regarding abs path is also consistent what I have with std::filesystem in c++17 (which we be nice if we could use, but unfortunately for the moment we have to stick to c++11) auto p = std::filesystem::path {"/foo"} / "/bar"; assert (p == "/bar"); If you compare to one of the testcase you add, the behavior is different: s = ::path_join ("/foo", "/bar", nullptr); SELF_CHECK (s == "/foo/bar"); Is this different semantics made on purpose? Note that there is another part of the behavior which is different in your proposal v.s. python and std::filesystem. Appending an empty component at the end ensures that the path terminates with a path separator: >>> os.path.join("foo", "") "foo/" If I use your patch, I see: s = ::path_join ("foo", "", nullptr); SELF_CHECK (s == "foo"); Using similar semantics to std::filesystem might allow us to replace our implementation with the c++ standard library when we switch to c++17. On a different note, I am not sure I am a huge fan of the last nullptr argument which is required. You can achieve a similar interface except for this nullptr using light variadic templates (not too much, should be easy enough to maintain). Is this something you considered? One implementation could be: /* The real worker can be compiled in a .cc file if you wish. */ extern std::string path_join_1 (std::string a, std::string b); template std::string path_join (std::string a, std::string b) { return path_join_1 (a, b); } I do not pretend this is optimal (it is not). In this POC I pass all arguments as std::string and by value, this is probably not what you want to do in a production grade implementation, but this just gives the overall idea. What do you think? Best, Lancelot.