Use of an external variant implementation

Use of an external variant implementation

[Simon Marchi]

Hi,

It was suggested a few times that we backport listdcxx's implementation
of std::variant (available in C++ >= 17), so that we can use it in GDB,
a bit like we have done for std::string_view and std::optional.  This
would allow replacing unions with something that can hold non-POD types.

Could we instead use an existing implementation such as this one?

  https://github.com/mapbox/variant

I think it would be wasteful to spend some time writing an
implementation, when there's already one that seems very well tested and
documented.

The only downside I see is that the API doesn't seem to match
std::variant's API exactly.  For example, the standard version uses
std::visit, while that version uses an apply_visitor function.  But I am
pretty sure we would be able to make a little shim so that we are able
to use the standard version when __cplusplus is >= 2017, and that
version otherwise.

The license seems to be BSD, is there a problem with redistributing it
with GDB?

The idea would be to copy just the few relevant headers in our repo,
like we do for readline or other external software.  We could also use
a submodule, I don't really mind.

Simon

Re: Use of an external variant implementation

[Simon Marchi]

On 2021-06-23 3:10 p.m., Simon Marchi via Gdb-patches wrote:
> Hi,
> 
> It was suggested a few times that we backport listdcxx's implementation
> of std::variant (available in C++ >= 17), so that we can use it in GDB,
> a bit like we have done for std::string_view and std::optional.  This
> would allow replacing unions with something that can hold non-POD types.
> 
> Could we instead use an existing implementation such as this one?
> 
>   https://github.com/mapbox/variant
> 
> I think it would be wasteful to spend some time writing an
> implementation, when there's already one that seems very well tested and
> documented.
> 
> The only downside I see is that the API doesn't seem to match
> std::variant's API exactly.  For example, the standard version uses
> std::visit, while that version uses an apply_visitor function.  But I am
> pretty sure we would be able to make a little shim so that we are able
> to use the standard version when __cplusplus is >= 2017, and that
> version otherwise.
> 
> The license seems to be BSD, is there a problem with redistributing it
> with GDB?
> 
> The idea would be to copy just the few relevant headers in our repo,
> like we do for readline or other external software.  We could also use
> a submodule, I don't really mind.
> 
> Simon

Ping about this.  I will eventually start writing a proper patch for
this.  But if there is a fundamental problem about this approach, I'd
like to know right away so that I don't waste time on it.

Simon

Re: Use of an external variant implementation

[Tom Tromey]

Simon> Could we instead use an existing implementation such as this one?

Simon>   https://github.com/mapbox/variant

This one claims to be BSD-licensed, but I looked a bit more and one of
the header files has a different license:

https://github.com/mapbox/variant/blob/master/include/mapbox/recursive_wrapper.hpp

    // Original license:
    //
    // Copyright (c) 2002-2003
    // Eric Friedman, Itay Maman
    //
    // Distributed under the Boost Software License, Version 1.0. (See
    // accompanying file LICENSE_1_0.txt or copy at
    // http://www.boost.org/LICENSE_1_0.txt)

So this seems a bit odd.


In principle I think it's fine to include something like this.  Coming
up with a list of requirements off the top of my head, I'd say:

* Must be GPL compatible
* ... and be correct in its use of its own license
* Be as standards-compatible as possible
  (so when we update to a new version of C++ we can drop it)
* Not introduce any new dependencies

That's all I could think of, but maybe I missed some?

On our side I think the other thing to do is make sure the origin and
import instructions for any such code are clear.

Simon> The only downside I see is that the API doesn't seem to match
Simon> std::variant's API exactly.  For example, the standard version uses
Simon> std::visit, while that version uses an apply_visitor function.  But I am
Simon> pretty sure we would be able to make a little shim so that we are able
Simon> to use the standard version when __cplusplus is >= 2017, and that
Simon> version otherwise.

I'd be fine with this sort of thing.

Simon> The idea would be to copy just the few relevant headers in our repo,
Simon> like we do for readline or other external software.  We could also use
Simon> a submodule, I don't really mind.

It's hard to introduce a submodule for the first time.
Maybe we should keep avoiding it.

Tom