Variant class with full move support

I tried to write my own variant class, that is fully move-semantics enabled. WRT to implemented visitors, they don't require any policy and like to be derived from boost::static_visitor or to contain typedefed result_type. The result type of the visitor deduced by means of std::result_of and also can be prvalue, rref, lref or const lref. The only requirement is that all the returning types for all possible invocations of visitor must be of the same type exactly. It can be improved using std::common_type or something alike but better (to keep referenceness). Multivisitor provides the ability to pass some (non-visitable, i.e. not derived from variant<> class) of the arguments to visitor. It is very useful (for me).

Forward substitution of partial visitors (is a part of implementation details) of multivisitor is perfectly inlining. But backstroke is evidently no, due to type erasure points facing. It is non-avoidable anyways.

The following code (55 = 3125 functions generated for visitor functor) compiles 20.9s vs 51.2s for boost::variant:

struct P
    : boost::static_visitor
{

    template
    result_type
    operator () (T &&...) const
    {
        //return __PRETTY_FUNCTION__;
    }

};

int main()
{
    struct A {};
    struct B {};
    struct C {};
    struct D {};
    struct E {};
    using V = boost::variant;
    P p;
    V v0(A{}), v1(B{}), v2(C{}), v3(D{}), v4(E{});
    boost::apply_visitor(p, v0, v1, v2, v3, v4);
    return 0;
}

The compilation time of the code for my variant class significantly depends on lengths of symbol names (both on type names provided as template parameters to variant<>, and, say, on enclosing my variant machinery into own namespace). Dependence have nonlinear character and such behaviour is very strange. Definitely, there is the matter for issue. Compilation time for boost::variant does not depend on indicated above.

On my mind the never-empty-guarantee observed completely.

Following code works on

The things you did right

Almost all of your code:

• Correct use of rvalue references, move semantics and perfect forwarding.

• You provided namespace-level functions that help with ADL (even though namespace-level get will probably not be found by ADL, but you can't do anything about that).

• You ensured that swap was noexcept.

• You gave meaningful names to your types and variables.

• You did some "concept check" with static_assert, which helps avoiding horrible error messages.

Basically, what you did is really good, especially since your edit. There's almost nothing left to say. Almost...

Naming stuff


There are only two hard things in Computer Science: cache invalidation, naming things and of-by-one errors.

Your variable names have generally good names. However, for some reason, you seem to hate capital letters. You could use some, at least for you template parameter names. That would also allow you to drop many underscores. This piece of code:

template
static
result_of, arguments... >
caller(storage && _storage, visitor && _visitor, arguments &&... _arguments)
{
    // ...
}

would probably be easier to read written as:

template
static
result_of, Arguments... >
caller(Storage && storage, Visitor && visitor, Arguments &&... arguments)
{
    // ...
}

I also almost got lost when I read this:

which_ = _which;

I understand the logic (leading underscore for class members), but I think that I would still make errors hard to detect if I wrote code with names so close from each other.

Exceptions

While I like the name bad_get for your exception (close to the names of the exceptions in the standard library), there are still some things that you can change in that class so that it looks even more like a standard library exception:

• You can make the destructor explicitly virtual.

• You can add a std::string overload to the constructor.

-
You could also add an explicit override qualifier to the method what to clarify your intent (even though the standard library does not do it):

virtual const char * what() const noexcept override { /* .... */ }

I have to admit that there are many keywords on the same line. You can find information about why you should use the override keyword here. But basically, remember that sometimes, you want to override a function in a derived class, but write a function with a slightly different signature. Sometimes, this kind of error can be really hard to spot (because of implicit base class name hiding for example). If you add override to make it clear that you intend to override a base class function, the compiler will tell you if you messed the signature and ended up not overriding anything. In short, it helps to prevent silent errors.

Dead code

You have some pieces of dead code left:

/*static_assert(std::is_lvalue_reference::value || !std::is_rvalue_reference::value,
              "visitor is not lvalue reference or value");*/

You should simply remove it. Dead code will not help you, and revision control software can help you to find old code that you may need but already removed.

Simplify your return statements

You can use list initialization to make some of your return statements more readable provided the return type is already known by the function:

template
constexpr
details::delayed_visitor_applier
apply_visitor(visitor && _visitor)
{
    return details::delayed_visitor_applier(std::forward(_visitor));
}

The code above can be reduced to:

template
constexpr
details::delayed_visitor_applier
apply_visitor(visitor && _visitor)
{
    return { std::forward(_visitor) };
}

That said, list initialization won't work if the constructor of details::delayed_visitor_applier is marked explicit.

StackExchange Code Review Q#48471, answer score: 9