5
\$\begingroup\$

Inspired by this cppcon talk from Fedor Pikus about design patterns and some real-world need I implemented a library that allows the user to add a generic visitor pattern capabilities to class hierarchies based on vanilla C++ runtime polymorphism with virtual functions. There were 2 requirements I wanted to follow:

  1. Generic return type, the same as when using std::visit on a std::variant.
  2. Allow as much forward declarations as reasonably possible, to decouple the client from any code (i.e. concrete implementations) that one doesn't need to know.

The code is on my github: https://github.com/ldrozdz93/cpp-visitor-pattern.

I'll also paste it here, as it's quite short.

The test file:

#include <string>
#include <type_traits>

#include "catch.hpp"
#include "vstor/vstor.hpp"

namespace {

using vstor::Overloaded;
using vstor::VisitableFor;
using vstor::VisitableImpl;
using vstor::VisitableListVariant;

// NOTE: below code order is important from the testing POV

// GIVEN a forward-declared list of visitables,
struct VisitablesVariant;

// WHEN creating a visitable base,
// THEN it can be declared with an incomplete type
struct Base : VisitableFor<VisitablesVariant> {
};

// AND list of visitables can be any template list
template <typename...>
struct AnyList;
struct D1;
struct D2;

using ListOfVisitables = AnyList<D1, D2>;

// AND require complete type only when visitation is used
struct VisitablesVariant : VisitableListVariant<ListOfVisitables> {
    using VisitableListVariant<ListOfVisitables>::VisitableListVariant;
};

template <typename Impl>
struct BaseImpl : VisitableImpl<Impl, Base> {
};

struct D1 : BaseImpl<D1> {
};
struct D2 : BaseImpl<D2> {
};

TEST_CASE("Visitor")
{
    using std::string;
    GIVEN("Polymorphic class hierarchy")
    {
        D2 d2{};
        WHEN("object is mutable")
        {
            Base& b = d2;
            THEN("can be visited")
            {
                string res =
                    b.visit_by(Overloaded{[](D1&) { return "D1"; }, [](D2&) { return "D2"; }});
                REQUIRE(res == "D2");
            }
        }
        WHEN("object is const")
        {
            const Base& b = d2;
            THEN("can be visited")
            {
                string res = b.visit_by(
                    Overloaded{[](const D1&) { return "D1"; }, [](const D2&) { return "D2"; }});
                REQUIRE(res == "D2");
            }
        }
        WHEN("object is rvalue")
        {
            THEN("can be visited")
            {
                // TODO: should non-const references be allowed to bind to rvalues?
                string res =
                    D1{}.visit_by(Overloaded{[](D1&) { return "D1"; }, [](D2&) { return "D2"; }});
                REQUIRE(res == "D1");
            }
        }

        // TODO: will constexpr ever work here?
        //        WHEN("object is constexpr")
        //        {
        //            constexpr const D1 d1{};
        //            THEN("can be visited")
        //            {
        //                constexpr int res = d1.visit_by(
        //                    overloaded{[](const D1&) { return 1; }, [](const D2&) { return 2; }});
        //                STATIC_REQUIRE(res == "D2");
        //            }
        //        }
    }
}

TEST_CASE("Utilities")
{
    SECTION("Overloaded")
    {
        // TODO: add better tests.

        using namespace std::string_literals;
        auto sut = vstor::detail::Overloaded{
            [](char) { return "char"s; },
            [](int) { return "int"s; },
            [](float) { return "float"s; },
        };
        REQUIRE("char" == sut(char{}));
        REQUIRE("int" == sut(int{}));
        REQUIRE("float" == sut(float{}));
    }
}

}  // namespace

And the implementation file:

#ifndef VSTOR_HPP
#define VSTOR_HPP

#include <type_traits>
#include <variant>

namespace vstor {

namespace detail {

/**
 * A commonly used template for creating lambda-based callable overload sets.
 *
 * @todo add possibility to set a fixed required return type
 * */
template <class... Ts>
class Overloaded : public Ts... {
public:
    using Ts::operator()...;
};
#if __cpp_deduction_guides < 201907L  // if CTAD for aggregates and aliases not supported
template <class... Ts>
Overloaded(Ts...) -> Overloaded<Ts...>;
#endif

// TODO: implement concept: invocable_with_each_variant_option<F, VisitablesVariant>

/**
 * A tag class for marking every VisitableFor<...> instance. Used instead of clunky sfinae for
 * checking if a given type is a subclass of an instantiation of VisitableFor<...> template.
 * */
struct VisitableFor_BaseTag {
};
template <typename T>
concept AnyVisitableFor = std::is_base_of_v<VisitableFor_BaseTag, T>;

/**
 * A base class for all Visitable classes, that perform the double dispatch of the visitor pattern.
 * The concrete implementations of this base participate in the first dispatch by virtual functions.
 * The second dispatch is performed as a std::visit visitation of possible visitables.
 *
 * @tparam Variant - a maybe-incomplete class, that at the point of instantiation must be a class
 * derived from VisitableListVariant<...>. Note it's not checked by any concept, because that would
 * require an eager instantiation of the list of possible visitable variants, which is not intended.
 * */
template <typename Variant>
class VisitableFor : public VisitableFor_BaseTag {
public:
    using VisitableVariant = Variant;

    virtual VisitableVariant as_variant() const noexcept = 0;

    /**
     * Performs visitation on 'this' by the same rules as std::visit does.
     */
    template <typename Visitor>
    // requires invocable_with_each_variant_option<Visitor, VisitableVariant>
    decltype(auto) visit_by(Visitor&& visitor);

    template <typename Visitor>
    // requires invocable_with_each_variant_option<Visitor, VisitableVariant>
    decltype(auto) visit_by(Visitor&& visitor) const;
};

template <typename Variant>
template <typename Visitor>
decltype(auto) VisitableFor<Variant>::visit_by(Visitor&& visitor)
{
    auto invoke_visitor_after_dereference_and_deconst = [&](auto&& v) -> decltype(auto) {
        // NOTE: it's safe to const_cast, because the pointee of the pointer inside the visitable
        // variant is in fact 'this', so it has the same cv-qualification
        using NonConstVisitable = std::remove_cvref_t<decltype(*std::forward<decltype(v)>(v))>;
        return std::invoke(std::forward<Visitor>(visitor),
                           const_cast<NonConstVisitable&>(*std::forward<decltype(v)>(v)));
    };
    // NOTE: the whole pattern cannot be noexcept friendly due to std::visit possibly throwing
    return std::visit(invoke_visitor_after_dereference_and_deconst, as_variant().as_std_variant());
}

template <typename Variant>
template <typename Visitor>
decltype(auto) VisitableFor<Variant>::visit_by(Visitor&& visitor) const
{
    auto invoke_visitor_after_dereference = [&](auto&& v) -> decltype(auto) {
        return std::invoke(std::forward<Visitor>(visitor), *std::forward<decltype(v)>(v));
    };

    return std::visit(invoke_visitor_after_dereference, as_variant().as_std_variant());
}

/**
 * Base class for the concrete visitable implementation.
 *
 * @tparam CrtpImpl - CRTP class that must be a part of the previously-defined VisitableVariant.
 * It's incomplete due to CRTP usage.
 *
 * @tparam Base - The concrete VisitableFor<...> base class this implementation is based on.
 * */
template <typename CrtpImpl, AnyVisitableFor Base>
class VisitableImpl : public Base {
public:
    using VisitableVariant = typename Base::VisitableVariant;

    /**
     * This method is const, without any non-const overload for the following reason: the
     * return value can be incomplete until used. The return value for a non-const overload would
     * need to differ from the const overload. Doing that with type traits would require a complete
     * VisitableVariant class, which we want to avoid, so the end user would need to provide 2
     * incomplete VisitableVariants that would differ from each other only with the possible variant
     * being const. Instead, we const_cast the const away in case we know the instance being worked
     * with is in fact non-const.
     *
     * @returns 'this' as one of the possible variant options.
     * */
    VisitableVariant as_variant() const noexcept final
    {
        return VisitableVariant{static_cast<const CrtpImpl*>(this)};
    }
};
template <typename... Args>
using StdVariantBase = std::variant<const Args* const...>;

/**
 * Base class for aggregating maybe-incomplete classes possible for visitation.
 *
 * @tparam Visitables - a variadic list of maybe-incomplete visitables.
 * */
template <typename... Visitables>
class VisitableListVariant : private StdVariantBase<Visitables...> {
public:
    using VariantBase = StdVariantBase<Visitables...>;
    using VariantBase::VariantBase;
    VariantBase& as_std_variant() & noexcept { return *this; }
    const VariantBase& as_std_variant() const& noexcept { return *this; }
};

/**
 * Specialization of VisitableListVariant that unfolds any list-like template into a proper
 * VisitableListVariant. It's intended to be used in case the user is keeping the visitable classes
 * as some templated list for any reason.
 *
 * @tparam AnyList - any template in a form of TemplateName<T1, T2, ...>,
 * ex: 'std::variant<T1, T2>' or 'boost::mpl::list<T1, T2>'
 * */
template <template <typename...> typename AnyList, typename... Visitables>
class VisitableListVariant<AnyList<Visitables...>> : public VisitableListVariant<Visitables...> {
public:
    using VisitableListVariant<Visitables...>::VisitableListVariant;
};

}  // namespace detail

using detail::Overloaded;
using detail::VisitableFor;
using detail::VisitableImpl;
using detail::VisitableListVariant;
}  // namespace vstor

#endif  // VSTOR_HPP

I'd value any concrete feedback.

\$\endgroup\$
1
\$\begingroup\$
// Specialization of VisitableListVariant that unfolds any list-like template

You don't want this. Imagine what happens when someone inherits from VisitableListVariant<std::string, int> versus when they inherit from VisitableListVariant<std::string> alone. (Recall that std::string is of the form TT<char, std::char_traits<char>, std::allocator<char>>; so it will match this specialization.)

You don't need this specialization — parameter packs can be passed around without wrapping, just fine. And it's harmful. So eliminate it.

Your test code is crazy convoluted. With all the comments removed, it's like this:

struct VisitablesVariant;
struct Base : VisitableFor<VisitablesVariant> {};
template <typename...> struct AnyList;
struct D1;
struct D2;
using ListOfVisitables = AnyList<D1, D2>;

struct VisitablesVariant : VisitableListVariant<ListOfVisitables> {
    using VisitableListVariant<ListOfVisitables>::VisitableListVariant;
};

template <typename Impl> struct BaseImpl : VisitableImpl<Impl, Base> {};
struct D1 : BaseImpl<D1> {};
struct D2 : BaseImpl<D2> {};

Disentangled, IIUC, it'd be like this:

struct D1;
struct D2;
struct Base : VisitableFor<VisitableListVariant<D1, D2>> {};
struct D1 : VisitableImpl<D1, Base> {};
struct D2 : VisitableImpl<D2, Base> {};

And I would rather see something like this:

struct D1;
struct D2;
struct Base : Visitable<D1, D2> {};
struct D1 : Base {};
struct D2 : Base {};

Implementation of this "cleaner" design is left as an exercise for the reader.


If you're using this only for visiting polymorphic hierarchies, you might be interested in this alternative design: https://quuxplusone.github.io/blog/2020/09/29/oop-visit/

struct Base { virtual ~Base() = default; };
struct D1 : Base {};
struct D2 : Base {};

const Base& b = d2;
std::string_view sv = my::visit<D1, D2>(b, Overloaded{
    [](const D1& d1) { return "d1"; },
    [](const D2& d2) { return "d2"; },
});
assert(sv == "d2");
\$\endgroup\$
6
  • \$\begingroup\$ Thanks for the feedback! That's a valid point about list-like specializations being harmful. \$\endgroup\$
    – lucas93
    Dec 28 '20 at 12:33
  • \$\begingroup\$ > Disentangled, IIUC, it'd be like this: > ``` > ... > struct Base : VisitableFor<VisitableListVariant<D1, D2>> {}; > ... > ``` I'm afraid you've not understood that correctly. Such a design would require the base declaration to know the full declaration of the list of all its visitables, which is not intended. That's why VisitablesVariant is forward-declared for the Base declaration. The test comments are clunky, but try to state that requirement. It's hard to express the requirements of physical design in unit tests, but I might improve it, so thanks for showing it's not obvious. \$\endgroup\$
    – lucas93
    Dec 28 '20 at 12:36
  • \$\begingroup\$ Thanks for linking the blog post. Great stuff. I'll compare to your approach. From the first look, from the physical design perspective your solution looks as decoupled as I wanted it to be in the call site. However, you allow for cast errors in runtime, which I wanted disallowed by compilation. \$\endgroup\$
    – lucas93
    Dec 28 '20 at 14:12
  • \$\begingroup\$ I'm also interested how the performance would compare between your and mine approach. I'll add some tests when I find a minute. \$\endgroup\$
    – lucas93
    Dec 28 '20 at 14:13
  • 1
    \$\begingroup\$ Okay, I've played with it in Godbolt a bit. godbolt.org/z/ofq986 Take a look at my rearranging of the test's pieces (I still think your way is "tangled" and I'm "disentangling" it), and also IMO I've improved the indentation on the Overloaded stuff. (I also had to #include <functional>, and declare the deduction guide unconditionally for GCC's benefit.) \$\endgroup\$ Dec 28 '20 at 18:47

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.