Re: [PATCH 5/5] gdbsupport: add path_join function

[Lancelot SIX <lsix@lancelotsix.com>]

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 <typename ...Args)
    std::string
    path_join (std::string a, std::string b, Args... comps)
    {
      return path_join (path_join (a, b), comps...);
    }

    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.