mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 100 forks source link

move out (via @lightmare) #180

Closed artemp closed 4 years ago

artemp commented 4 years ago

Based on https://github.com/mapbox/variant/pull/178 /cc @springmeyer

artemp commented 4 years ago

@lightmare - I stumble on issue in your PR where wrong visit(V const&, F&&) overload is chosen by compiler (tested with clang10 and gcc9.3). /cc @springmeyer

#include <mapbox/variant.hpp>
#include <iostream>
#include <type_traits>

namespace {

struct object
{
    void foo() { val = 42;}
    int val;
};

struct visitor
{
    template <typename T>
    void operator() (T & obj) const
    {
        std::cerr << typeid(obj).name() << std::endl;
        obj.foo();
    }
};

}

int main()
{
    using variant_type = mapbox::util::variant<object>;
    variant_type v = object{};
    mapbox::util::apply_visitor(visitor(), v); // OK

    mapbox::util::apply_visitor([](auto& obj)
                                {
                                    obj.foo(); // <-- compilation failure
                                    // const_cast<object&>(obj).foo(); // hack
                                }, v);
    return 0;
}
lightmare commented 4 years ago

@artemp thanks for the test case. I found it easier to fix than to understand why it was failing.

The test and fix are pushed on top of my original PR. It's fairly simple, instead of 3 visit templates matching specific variant reference types, there's just one visit template that takes a forwarding reference to the variant and transfers its const and ref qualifiers to the first alternative type.

Now on to explain what's going on in the failing test case. It looks like the visitor class and the lambda expression's closure class have almost identical operator(), so it's perplexing that one works and the other doesn't.

But there is a difference. The visitor's operator() is declared with a return type, while the lambda's return type must be inferred from (lack of) return statements. My hunch is that this forces the compiler to instantiate the body of the lambda when considering each of the 3 visit templates, and then it rightfully complains that the instantiation required by visit(variant const&, ...) calls non-const foo on a const object. If you add trailing return type -> void to the lambda expression, it will compile, because visit(const& variant, ...) will be ruled out by SFINAE without instantiating the lambda's body. Likewise, if you change the declaration in the visitor class to auto operator()(...), it will fail with the same error as the lambda without trailing return type.

artemp commented 4 years ago

@lightmare thanks for the fix and info!

    mapbox::util::apply_visitor([](auto& obj) -> void
                                {
                                    obj.foo();
                                }, v);

^ specifying return type in lambda works with original 3 visit()'s but I prefer your latest fix.