felixguendling / cista

Cista is a simple, high-performance, zero-copy C++ serialization & reflection library.
https://cista.rocks
MIT License
1.8k stars 116 forks source link

Support for std::visit #205

Closed opera-aberglund closed 9 months ago

opera-aberglund commented 9 months ago

When using the cista variant, it would be really nice to be able to apply a visitor on the object like std::visit in the C++ standard library.

This to avoid having to add multiple if-statements checking holds_alternative on each type in the variant and improve performance.

felixguendling commented 9 months ago

variant.apply should work?

opera-aberglund commented 9 months ago

variant.apply should work?

Oh I totally missed that method. It worked perfectly, thanks!

ChemistAion commented 6 months ago

@felixguendling ...rediscovering this gem once again :sweat_smile: (after some break with cista) IMHO it would be helpful if the docu provided some basic description about the variant/visit directly, for the posterity :unlock:

felixguendling commented 6 months ago

I just added support for std::visit, so it should work now like with std::variant for the special case of exactly one cista::variant which is probably what most people use. Let me know here or make a PR to change the behavior.

ChemistAion commented 6 months ago

@felixguendling Thank you for prompt action here, yes - works like a charm.

Perhaps adding this common helper around or next to cista::variant (or embedding it into variant::apply) would simplify std::visitor and variant::apply for cases involving more than (let's say) three types in a variant. This could help avoid a 'christmas-tree' scenario of if constexpr statements.

template<class... Types>
struct Overloaded : Types...
{
    using Types::operator()...;
};

template<class... Types>
Overloaded(Types...) -> Overloaded<Types...>;

Since then, IMHO we have established a clear and readable usage pattern, even when lambdas would be just forwarded into it:

struct TestNodeA {};
struct TestNodeB {};
struct TestNodeC {};

using TestNodes = cista::variant<TestNodeA, TestNodeB, TestNodeC>;
TestNodes test { TestNodeB{} };

//////////////////////////////

std::visit
(
    Core::Overloaded
    {
        [](const TestNodeA&) { std::cout << "TestNodeA" << std::endl; },
        [](const TestNodeB&) { std::cout << "TestNodeB" << std::endl; },
        [](const TestNodeC&) { std::cout << "TestNodeC" << std::endl; }
    },
    std::forward<TestNodes>(test)
);

// ...or even more elegantly, just with cista::variant.apply()

test.apply
(
    Core::Overloaded
    {
        [](const TestNodeA&) { std::cout << "TestNodeA" << std::endl; },
        [](const TestNodeB&) { std::cout << "TestNodeB" << std::endl; },
        [](const TestNodeC&) { std::cout << "TestNodeC" << std::endl; }
    }
);

//////////////////////////////
felixguendling commented 6 months ago

Yes, we're also using this everywhere together with variant: https://github.com/motis-project/utl/blob/master/include/utl/overloaded.h

Not sure, if cista needs it as I feel like everyone is already using it since it's part of the "official" std::visit documentation: https://en.cppreference.com/w/cpp/utility/variant/visit

opera-aberglund commented 6 months ago

Should honestly be part of the standard library, std::overload. I agree Cista doesn't need it as long as it can accomplish essentially the same functionality as std::variant and its related functions

ChemistAion commented 6 months ago

You both were too fast! 😆 Indeed, this should be part of the standard, no need to fluff it up here. IMHO the comments above are more than enough for this matter... let's consider it as done (as-is).