non-static variant::visit

[lightmare]

I'm creating this separate issue to keep the discussion in #113 clean.

@artemp Because variant::visit doesn't modify internal state and making it static gives compiler some hints on how to generate binary code. Why are you thinking it shouldn't be static ?

What hints? Visiting a non-const variant can modify internal state (e.g. comparison operators can give different results after such visit; edit: that's modifying external state, actually).

visit is a static member function template only by syntax. Its first argument is always a reference to the class it's defined in. That's identical to non-static member functions under the hood.

Why this:

template <typename F, typename V>
auto VARIANT_INLINE apply_visitor(F&& f, V& v) -> decltype(V::visit(v, std::forward<F>(f)))
{
    return V::visit(v, std::forward<F>(f));
}

instead of this:

template <typename F, typename V>
auto VARIANT_INLINE apply_visitor(F&& f, V& v) -> decltype(v.visit(std::forward<F>(f)))
{
    return v.visit(std::forward<F>(f));
}

// ignoring for now that V should be forwarded as well

[artemp]

@lightmare - ok, in this particular case both version will be likely inlined so not much difference, but I'm personally considering current version a better coding style. Why are you so concerned about this ?

That's identical to non-static member functions under the hood.

^ Did you compare assembly code generated by compilers to justify such an assertion ? :)

[lightmare]

I'm personally considering current version a better coding style.

I think it's the exact opposite. Let me compare this to vector swap from libc++ for example:

template <class _Tp, class _Allocator>
inline _LIBCPP_INLINE_VISIBILITY
void
swap(vector<_Tp, _Allocator>& __x, vector<_Tp, _Allocator>& __y)
    _NOEXCEPT_(_NOEXCEPT_(__x.swap(__y)))
{
    __x.swap(__y);

    // having static vector::swap would turn the above into
    vector<_Tp, _Allocator>::swap(__x, __y);
}

It's the same thing as apply_visitor, free function taking an instance and delegating the work to a member function. Declaring variant::visit static is wrong, it's not a class method in OO terms.

:smile_cat: Now I realized I didn't even need to go to libc++. I could've used mapbox::util::get as an example. It delegates to var.template get<R>(), not T::template get<R>(var).

Did you compare assembly code generated by compilers to justify such an assertion ? :)

I assumed that'd be a waste of time. Yes, assembly generated by {g++-6,clang-3.8} -O{0..2} with current and modified variant are identical.

[lightmare]

Here's another argument: static variant::visit is essentially an implementation detail. If it were part of the public interface, it'd be a terrible one.

auto x = gimmeVariant(42);
x.visit(x, visitor); // yuck

It's just a weird way to apply_visitor. Make it non-static, or get rid of it.

[springmeyer]

varian::visit is used in mbgl. @jfirebaugh will likely want to keep that interface or have other thoughts.

[jfirebaugh]

I prefer writing MyType::visit(instance, visitor) to mapbox::util::apply_visitor(instance, visitor), because it's shorter and doesn't make me use a foreign naming convention in my otherwise camel case code. But my preferred syntax would be instance.accept(visitor).

[lightmare]

But my preferred syntax would be instance.accept(visitor).

+1