Re: [Patch] Implement std::experimental::variant

[Jonathan Wakely <jwakely at redhat dot com>]

On 05/08/16 22:45 -0700, Tim Shen wrote:
> On Fri, Aug 5, 2016 at 4:08 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> > I think these all need to use ::new (__ptr) with qualification, see
> > below.
>
> Done. I didn't add a testcase for this. Do you think that we need one/some?

No, I don't think we need a test. I doubt anybody will try to remove
the qualification, and if they do somebody who understands the need
for it can explain it before the patch gets committed :-)

> > > +  // Stores a void alternative, until void becomes a regular type.
> >
> >
> > Personally I hope it won't become a regular type :-)
>
> Sorry :)
>
> Do you mind to share your insights?

My impression of the proposal was that making it regular solves a few
problems, but introduces some other incompatibilities. If we were
designing a new language it might be a reasonable approach, but I
wouldn't want to introduce such a different object model based on what
we have today.


> > (The decval<void>() case is OK as that won't do ADL).
>
> Done for all of them, for consistency.
>
> > > +  template<typename _Array_type, typename _First, typename... _Rest,
> > > +          typename... _Args>
> > > +    struct __gen_vtable_impl<_Array_type, tuple<_First, _Rest...>,
> > > +                            tuple<_Args...>>
> >
> >
> > Do you actually need to use std::tuple here, or would something much
> > more lightweight be OK too?
>
> I actually just need a template to hold all alternatives. How about
> forward declaring tuple, and not including the header?
>
> I was totally unaware of the cost of <tuple>, and tried to use
> tuple_cat() on a bunch of function calls:
> (void)tuple_cat(foo<_Types>()...) where foo returns tuple<>.
>
> But not we have fold expression \o/! So I can directly write:
> (foo<_Types>, ...)
>
> which is perfect.

Nice.

> > For example we have std::tr2::__reflection_typelist in
> > <tr2/type_traits>. Or it looks like this could just use something even
> > simpler:
> >  template<typename... T> struct __type_list
> >
> > What we really need is to standardize Eric Niebler's metapgroamming
> > library, or Peter Dimov's one, or *anything* that gives us a set of
> > nice tools for doing this stuff. std::tuple is a very heavyweight type
> > to use for simple type lists.
>
> I don't need the whole pack of metaprogramming tools here, lucky me. ;)
>
> > It doesn't look like you're using any members of std::tuple, so maybe
> > it won't actually instantiate any of the base classes or member
> > functions, which is what would be inefficient.
> >
> > > +  template<typename _Tp, typename... _Types>
> > > +    _Tp& get(variant<_Types...>& __v)
> >
> >
> > Please add 'inline' to these one-line functions. All the get and
> > get_if overloads are tiny.
>
> What's the difference between non-inline function templates and inline
> function templates? At some point you may already explained that to
> me, but I'm still confused.

In practical terms, there's no difference. Function templates and
inline functions both get compiled to weak symbols, which are allowed
to have multiple definitions. Duplicate definitions will be discarded
by the linker.

That's an implementation detail related to how they are handled by the
compiler.  In formal C++ language terms they are still different
things. A function template is not an inline function unless it is
declared 'inline'. My preference is to add 'inline' if it's a small
function that would typically be marked inline if it wasn't a
template.

If the compiler ever uses the presence of the 'inline' keyword as a
hint for inlining (rather than just as the trigger for generating a
weak symbol) then putting it there would make a difference.

> Good news! This compiles now! I learned the technique from Anthony
> Williams's implementation, whose code also compiles, but it requires a
> close-to-trunk gcc, which implements
> "...for unions, at least one non-static data member is of non-volatile
> literal type, ...".
>
> Also added it as a test.

Great.

> Please verify the implementation by looking at _Uninitialized and
> _Variant_storage.
>
> > However, I think we could commit this for now as it's 99% complete.
> > What do you think?
> >
> > I am concerned that if we commit this implementation now, and *don't*
> > get a 100% conforming rewrite before we want to declare C++17 support
> > stable and non-experimental, then we'd have to introduce an
> > incompatible change. That could be done by replacing std::variant with
> > std::_V2::variant, so it would still be possible.
> >
> > If you want to propose this for trunk please make the fixes noted
> > above and send a new patch, CCing gcc-patches.
> >
> > Thanks!
>
> I also moved the tests from experiemntal/variant to 20_util/variant.
>
> Bootstrapped and tested on x86_64-linux-gnu.

OK for trunk, thanks!