11
\$\begingroup\$

As part of learning C++, with special emphasis on C++11, I wanted to implement the equivalent of Boost's Variant (located here). My code is available at variant.hpp, with the current version given below.

How can std::aligned_storage be used portably? My current solution makes probably non-portable use of static_cast, though if it is portable, that information would be very valuable. The particular code is similar to *static_cast<T*>(static_cast<void*>(&value)), for value of type typename std::aligned_storage<...>::type (where ... is not meant to indicate variadic templates).

I make some use of static_assert. In this particular use, would SFINAE be better? I understand SFINAE can be used to prune out overloads from the set of viable functions, but where I use static_assert I assume there would be only one viable function, though I would find valuable any examples of cases where there is more than one viable function.

I made much use of std::forward. Is it possible to get by with fewer uses?

I made use of std::enable_if on one of the constructor overloads to ensure that it would only be used when a move is intended (see variant(U&& value, typename detail::variant::enable_if_elem<U, T...>::type* = nullptr, typename detail::variant::enable_if_movable<U>::type* = nullptr)). Without both of the enable_ifs, this contructor would be used when the copy constructor variant(variant const&) is instead intended, even though the former results in an eventual compiler error. Is there a better way to force this behavior? One solution I tried was including variant(variant&) as an overload that just delgates to variant(variant const& rhs) - it would be selected over variant(U&&), while variant(U&&) is preferred over variant(variant const&) by the overload rules. What is the general best practice when using T&& for some newly introduced T when move semantics, instead of a universal reference, are intended?

I still need to add multivisitors, though I am having some trouble with this in the general case (using variadic templates). Something interesting that came up when implementing the variant class was implicit conversions between variants that only involved rearranging the template arguments or where the lvalue template arguments are a superset of the rvalue template arguments.

Any and all comments, questions, or advice is very much appreciated.

#ifndef WART_VARIANT_HPP
#define WART_VARIANT_HPP

#include <type_traits>
#include <utility>

#include "math.hpp"

namespace wart {
  template <typename... T>
  class variant;

  namespace detail {
    namespace variant {
      template <typename... T>
      using variant = wart::variant<T...>;

      template <typename T>
      using is_movable = typename std::integral_constant
        <bool,
         std::is_rvalue_reference<T&&>::value && !std::is_const<T>::value>;

      template <typename T, typename U = void>
      using enable_if_movable = std::enable_if<is_movable<T>::value, U>;

      template <typename... Types>
      using union_storage = typename std::aligned_storage
        <math::max_constant<std::size_t,
                            sizeof(Types)...>::value,
         math::lcm_constant<std::size_t,
                            std::alignment_of<Types>::value...>::value>::type;

      template <typename... Types>
      using union_storage_t = typename union_storage<Types...>::type;

      template <typename Elem, typename... List>
      struct elem;

      template <typename Head, typename... Tail>
      struct elem<Head, Head, Tail...>: std::true_type {};

      template <typename Elem, typename Head, typename... Tail>
      struct elem<Elem, Head, Tail...>: elem<Elem, Tail...>::type {};

      template <typename Elem>
      struct elem<Elem>: std::false_type {};

      template <typename Elem, typename... List>
      struct elem_index;

      template <typename Head, typename... Tail>
      struct elem_index<Head, Head, Tail...>:
        std::integral_constant<int, 0> {};

      template <typename Elem, typename Head, typename... Tail>
      struct elem_index<Elem, Head, Tail...>:
        std::integral_constant<int, elem_index<Elem, Tail...>::value + 1> {};

      template <bool... List>
      struct all;

      template <>
      struct all<>: std::true_type {};

      template <bool... Tail>
      struct all<true, Tail...>: all<Tail...>::type {};

      template <bool... Tail>
      struct all<false, Tail...>: std::false_type {};

      template <typename Elem, typename... List>
      using enable_if_elem = std::enable_if<elem<Elem, List...>::value>;

      template <typename F, typename... ArgTypes>
      using common_result_of =
        std::common_type<typename std::result_of<F(ArgTypes)>::type...>;

      struct destroy {
        template <typename T>
        void operator()(T&& value) {
          using type = typename std::remove_reference<T>::type;
          std::forward<T>(value).~type();
        }
      };

      struct copy_construct {
        void* storage;
        template <typename T>
        void operator()(T const& value) {
          new (storage) T(value);
        }
      };

      template <typename... T>
      struct copy_construct_index {
        void* storage;
        template <typename U>
        int operator()(U const& value) {
          new (storage) U(value);
          return elem_index<U, T...>::value;
        }
      };

      struct move_construct {
        void* storage;
        template <typename T>
        typename enable_if_movable<T>::type operator()(T&& value) {
          new (storage) T(std::move(value));
        }
      };

      template <typename... T>
      struct move_construct_index {
        void* storage;
        template <typename U>
        typename enable_if_movable<U, int>::type operator()(U&& value) {
          new (storage) U(std::move(value));
          return elem_index<U, T...>::value;
        }
      };

      struct copy_assign {
        void* storage;
        template <typename T>
        void operator()(T const& value) {
          *static_cast<T*>(storage) = value;
        }
      };

      template <typename... T>
      struct copy_assign_reindex {
        variant<T...>& variant;
        template <typename U>
        void operator()(U const& value) {
          if (variant.which_ == elem_index<U, T...>::value) {
            *static_cast<U*>(static_cast<void*>(&variant.storage_)) = value;
          } else {
            variant.accept(destroy{});
            new (&variant.storage_) U(value);
            variant.which_ = elem_index<U, T...>::value;
          }
        }
      };

      struct move_assign {
        void* storage;
        template <typename T>
        typename enable_if_movable<T>::type operator()(T&& value) {
          *static_cast<T*>(storage) = std::move(value);
        }
      };

      template <typename... T>
      struct move_assign_reindex {
        variant<T...>& variant;
        template <typename U>
        typename enable_if_movable<U>::type operator()(U&& value) {
          if (variant.which_ == elem_index<U, T...>::value) {
            *static_cast<U*>(static_cast<void*>(&variant.storage_)) = std::move(value);
          } else {
            variant.accept(destroy{});
            new (&variant.storage_) U(std::move(value));
            variant.which_ = elem_index<U, T...>::value;
          }
        }
      };
    }
  }

  template <typename... T>
  class variant {
    int which_;
    detail::variant::union_storage_t<T...> storage_;

  public:
    template <typename F>
    using result_of = detail::variant::common_result_of<F, T...>;
    template <typename F>
    using result_of_t = typename result_of<F>::type;

    template <typename U>
    variant(U const& value,
            typename detail::variant::enable_if_elem<U, T...>::type* = nullptr):
      which_{detail::variant::elem_index<U, T...>::value} {
      new (&storage_) U(value);
    }

    template <typename U>
    variant(U&& value,
            typename detail::variant::enable_if_elem<U, T...>::type* = nullptr,
            typename detail::variant::enable_if_movable<U>::type* = nullptr):
      which_{detail::variant::elem_index<U, T...>::value} {
      new (&storage_) U(std::move(value));
    }

    variant(variant const& rhs):
      which_{rhs.which_} {
      rhs.accept(detail::variant::copy_construct{&storage_});
    }

    template <typename... U>
    variant(variant<U...> const& rhs,
            typename std::enable_if<
            detail::variant::all<detail::variant::elem<U, T...>::value...>::value
            >::type* = nullptr):
      which_{rhs.accept(detail::variant::copy_construct_index<T...>{&storage_})} {}

    variant(variant&& rhs):
      which_{rhs.which_} {
      std::move(rhs).accept(detail::variant::move_construct{&storage_});
    }

    template <typename... U>
    variant(variant<U...>&& rhs,
            typename std::enable_if<
            detail::variant::all<detail::variant::elem<U, T...>::value...>::value
            >::type* = nullptr):
      which_{std::move(rhs).accept(detail::variant::move_construct_index<T...>{&storage_})} {}

    ~variant() {
      accept(detail::variant::destroy{});
    }

    variant& operator=(variant const& rhs) & {
      using namespace detail::variant;
      static_assert(all<std::is_nothrow_copy_constructible<T>::value...>::value,
                    "all template arguments T must be nothrow copy constructible in class template variant");
      if (this == &rhs) {
        return *this;
      }
      if (which_ == rhs.which_) {
        rhs.accept(copy_assign{&storage_});
      } else {
        accept(destroy{});
        rhs.accept(copy_construct{&storage_});
        which_ = rhs.which_;
      }
      return *this;
    }

    template <typename... U>
    variant& operator=(variant<U...> const& rhs) & {
      using namespace detail::variant;
      static_assert(all<std::is_nothrow_copy_constructible<T>::value...>::value,
                    "all template arguments T must be nothrow copy constructible in class template variant");
      rhs.accept(copy_assign_reindex<T...>{*this});
      return *this;
    }

    variant& operator=(variant&& rhs) & {
      using namespace detail::variant;
      static_assert(all<std::is_nothrow_move_constructible<T>::value...>::value,
                    "all template arguments T must be nothrow move constructible in class template variant");
      if (this == &rhs) {
        return *this;
      }
      if (which_ == rhs.which_) {
        std::move(rhs).accept(move_assign{&storage_});
      } else {
        accept(detail::variant::destroy{});
        std::move(rhs).accept(move_construct{&storage_});
        which_ = rhs.which_;
      }
      return *this;
    }

    template <typename... U>
    variant& operator=(variant<U...>&& rhs) & {
      using namespace detail::variant;
      static_assert(all<std::is_nothrow_copy_constructible<T>::value...>::value,
                    "all template arguments T must be nothrow copy constructible in class template variant");
      std::move(rhs).accept(move_assign_reindex<T...>{*this});
      return *this;
    }

    template <typename F>
    result_of_t<F> accept(F&& f) const& {
      using namespace detail::variant;
      using call = result_of_t<F&&> (*)(F&& f, union_storage_t<T...> const&);
      static call calls[] {
        [](F&& f, union_storage_t<T...> const& value) {
          return std::forward<F>(f)(*static_cast<T const*>(static_cast<void const*>(&value)));
        }...
      };
      return calls[which_](std::forward<F>(f), storage_);
    }

    template <typename F>
    result_of_t<F> accept(F&& f) & {
      using namespace detail::variant;
      using call = result_of_t<F&&> (*)(F&& f, union_storage_t<T...>&);
      static call calls[] {
        [](F&& f, union_storage_t<T...>& value) {
          return std::forward<F>(f)(*static_cast<T*>(static_cast<void*>(&value)));
        }...
      };
      return calls[which_](std::forward<F>(f), storage_);
    }

    template <typename F>
    result_of_t<F> accept(F&& f) && {
      using namespace detail::variant;
      using call = result_of_t<F> (*)(F&& f, union_storage_t<T...>&&);
      static call calls[] {
        [](F&& f, union_storage_t<T...>&& value) {
          return std::forward<F>(f)(std::move(*static_cast<T*>(static_cast<void*>(&value))));
        }...
      };
      return calls[which_](std::forward<F>(f), std::move(storage_));
    }

    friend
    struct detail::variant::copy_assign_reindex<T...>;

    friend
    struct detail::variant::move_assign_reindex<T...>;
  };
}

#endif
\$\endgroup\$
  • \$\begingroup\$ Also per our help center, this is not the site for code that doesn't do what it is supposed to do. Does your code work? \$\endgroup\$ – Simon Forsberg Mar 2 '14 at 17:39
  • 4
    \$\begingroup\$ If by "work", you mean, "does it compile?", it compiles with clang++ version 3.3, but does not compile with g++ version 4.8.2 due to an interaction between parameter packs and lambdas. The CMakeLists.txt file selects clang++ unconditionally. If by "work", you mean "is it portable?", then no, it may not be portable. This is due to the way I use std::aligned_storage. Advice on portable use of the associated type would be very helpful. If by "work", you mean "does it satisfy the original intent?", the answer is yes, though I would very much like stylistic and best-practice advice. \$\endgroup\$ – ScootyPuff Mar 2 '14 at 21:17
  • \$\begingroup\$ The use of std::forward looks fine, I can't see any place where you could not use it. I prefer static_assert as it allows you to give better error messages. Portability is potentially a problem, as there is no guarantee all pointers are aligned on the same boundary (so void * and T* may have different requirements), but this would be very unusual these days. I don't know how boost gets around this, or if they just ignore it. \$\endgroup\$ – Yuushi Mar 5 '14 at 1:47
  • \$\begingroup\$ @Yuushi, first, thank you for taking the time to review this. I think they just ignore any non-portable stuff (boost.org/doc/libs/1_55_0/boost/variant/detail/cast_storage.hpp). I did find a portable solution (github.com/sonyandy/cpp-experiments/blob/master/include/wart/…) that uses a technique similar to what std::tuple uses, but with union. \$\endgroup\$ – ScootyPuff Mar 5 '14 at 3:14
  • \$\begingroup\$ No problem. The only other thing I'd add is that the file seems a bit "busy" and there are things in it that are potentially useful in their own right (traits that you've defined like is_movable and enable_if_movable, for example) that could potentially live somewhere else. Unfortunately, the number of C++ reviewers around here with the knowledge needed to give you good feedback on this is probably in the single digits, as the code is fairly complex. \$\endgroup\$ – Yuushi Mar 5 '14 at 3:21
2
\$\begingroup\$

There's a lot here, so I'm going to split my review into pieces. I want to start by just focusing on the metafunction section. Metafunctions may be short, but they're very powerful and important to get right - but in terms of correctness and usefulness.

To start with:

template <typename T>
using is_movable = typename std::integral_constant
    <bool,
     std::is_rvalue_reference<T&&>::value && !std::is_const<T>::value>;

template <typename T, typename U = void>
using enable_if_movable = std::enable_if<is_movable<T>::value, U>;

The first one is simply wrong. You're using this metafunction to check if a type is move constructible (in move_construct)... but you're doing this by just checking if it's neither an lvalue reference nor const. You're not actually checking anything relating to move construction. Just because something is an rvalue reference does not mean that you can move from it. And just because something is an lvalue reference does not mean that you cannot. Consider two simple classes:

struct No {
    A(A&& ) = delete;
};

struct Yes { };

As the name suggests, No is not move constructible. Your metafunction says it is. Also, Yes& is move constructible but your metafunction says no.

The correct implementation would be to simply use the standard type trait std::is_move_constructible.

Secondly, the alias there is questionable. Typically, we'd use aliases to avoid having to write the typename ::type cruft. You're not doing that, and the resulting call isn't that much more concise. Compare:

typename enable_if_movable<T>::type      // yours with alias
std::enable_if_t<is_moveable<T>::value>  // just using enable_if without alias
std::enable_if_t<std::is_move_constructible<T>::value> // just stds

I would prefer the last version personally. Note that I'm using the C++14 alias here. If you don't have a C++14 compiler, it is absolutely worth it to start your metafunction library with all of them. They are simply to write:

template <bool B, typename T = void>
using enable_if_t = typename std::enable_if<B, T>::type;

Moving onto:

template <typename Elem, typename... List>
struct elem;

There is no way that anybody will know what elem does here. I didn't until I read the implementation. A much better name for this would be contains. But I'll get back to implementation in a moment.

First, let's start with:

template <bool... List>
struct all;

all is super useful. So are its close relatives any and none. The way you wrote all is fine and works, but doesn't make it easier to write the other two. A good way of writing these out is to use @Columbo's bool_pack trick:

template <bool...> struct bool_pack;

template <bool f, bool... bs>
using all_same = std::is_same<bool_pack<f, bs...>, bool_pack<bs..., f>>;

That's just your helper. You can use that to implement all the rest easily:

template <bool... bs>
using all_of = all_same<true, bs...>;

template <bool... bs>
using none_of = all_same<false, bs...>;

template <bool... bs>
using any_of = std::integral_constant<bool, !none_of<bs...>::value>;

And once we have that, we can reimplement contains as a one-liner:

template <typename Elem, typename... List>
using contains = any_of<std::is_same<Elem, List>::value...>;

Similarly to before, I don't see the value in enable_if_elem. And common_result_of should take the type, not just yield the metafunction:

template <typename F, typename... ArgTypes>
using common_result_of =
    std::common_type_t<std::result_of_t<F(ArgTypes)>::...>;

Although it's more readabile to just stick that in your variant itself:

// no need to use anything in detail::, unless you need to 
// write your own aliases for common_type_t and result_of_t
template <typename F>
using result_of = std::common_type_t<std::result_of_t<F(T)>...>;

Now onto the usage. Throughout, you use these metafunctions in the return type:

template <typename T>
typename enable_if_movable<T>::type operator()(T&& value);

Or as a dummy pointer:

template <typename U>
variant(U const& value,
        typename detail::variant::enable_if_elem<U, T...>::type* = nullptr)

But in both cases, I find it a lot easier to parse complex template expressions if you put the SFINAE logic as an unnamed final template parameter:

template <typename T,
          typename = std::enable_if_t<std::is_move_constructible<T>::value>
          >
void operator()(T&& value);

template <typename U,
          typename = std::enable_if_t<contains<U, T...>::value>
          >
variant(U const& value);

The consistency helps understanding too. The dummy pointer argument is a confusing hack leftover from C++03. There's no need for it anymore. Especially when you need two dummy pointers:

 template <typename U,
           typename = std::enable_if_t<contains<U, T...>::value &&
                                       std::is_move_constructible<U>::value>
           >
 variant(U&& value);

Side note on this guy. It doesn't actually do what you want. This isn't an arbitrary rvalue reference - it's a forwarding reference. In fact, we can even combine the two constructors here in one go:

template <typename U,
          typename V = std::remove_reference_t<U>,
          typename = std::enable_if_t<contains<V, T...>::value &&
                                      std::is_constructible<V, U&&>::value>
          >
variant(U&& value)
: which_{elem_index<V, T...>::value}
{
    new (&storage_) V(std::forward<U>(value));  
}

Note that this also solves another problem with your code - namely that you never check if a class is copy constructible. What if you wanted to stick something like unique_ptr in your variant. Move constructible, but not copy constructible - but you never checked that in your code. Important - otherwise your users would just get cryptic error messages.

I think this concludes the metafunction portion. I will write a review of variant itself a bit later. Hope you find this helpful.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.