Support serialization and deserialization of std::variant

[anthonybrandon]

It can be useful to directly serialize from and deserialize to std::variant<> instead of sdbus::Variant.

[sangelovic]

Thanks for the PR, it's interesting and useful. I edited your code directly -- I've done some minor adjustments (formatting mostly, for consistency with existing code), and I've also simplified slightly the deserialization template so its invocation is simpler.

Out of curiosity, I also played with some implementation variations of that deserialization template. One alternative would be:

namespace detail
{
	template <typename _Element, typename _Variant>
	bool deserialize_variant(Message& msg, _Variant& value, const std::string& signature)
	{
		if (signature != signature_of<_Element>::str())
			return false;

		_Element temp;
		msg.enterVariant(signature);
		msg >> temp;
		msg.exitVariant();
		value = std::move(temp);
		return true;
	}

	template <typename _Variant, std::size_t... _Is>
	void deserialize_variant(Message& msg, _Variant& value, const std::string& signature, std::index_sequence<_Is...>)
	{
		bool result = (deserialize_variant<std::variant_alternative_t<_Is, _Variant>>(msg, value, signature) || ...);
		SDBUS_THROW_ERROR_IF(!result, "Failed to deserialize variant: signature did not match any of the variant types", EINVAL);
	}
}

template <typename... _Elements>
inline Message& Message::operator>>(std::variant<_Elements...>& value)
{
	std::string type;
	std::string contentType;
	peekType(type, contentType);
	detail::deserialize_variant(*this, value, contentType, std::index_sequence_for<_Elements...>{});
	return *this;
}

I like that this one is neat and compact, using fold expressions instead of recursion and template specialization. With C++20 we could use template lambda instead of bool deserialize_variant(...). But perhaps its function and intent may be not that revealing at the first look, making it more difficult to see what the code does...

The other would be:

namespace detail
{
	template <std::size_t _I = 0, typename... _Elements>
	void deserialize_variant(Message& msg, std::variant<_Elements...>& value, const std::string& signature)
	{
		if constexpr (_I < std::variant_size_v<std::variant<_Elements...>>)
		{
			using Element = std::variant_alternative_t<_I, std::variant<_Elements...>>;
			if (signature == signature_of<Element>::str())
			{
				Element temp;
				msg.enterVariant(signature);
				msg >> temp;
				msg.exitVariant();
				value = std::move(temp);
			}
			else
				deserialize_variant<_I + 1>(msg, value, signature);
		}
		else
			SDBUS_THROW_ERROR("Failed to deserialize variant: signature did not match any of the variant types", EINVAL);
	}
}

template <typename... _Elements>
inline Message& Message::operator>>(std::variant<_Elements...>& value)
{
	std::string type;
	std::string contentType;
	peekType(type, contentType);
	detail::deserialize_variant(*this, value, contentType);
	return *this;
}

Here I like that we have one function only, the invocation and the deserialization function are quite simple, and it's perhaps more readable and expressive what the code does (recursive "iteration" with incrementing index...). Here I also used std::variant directly, as it makes it clearer that the function is about std::variant.

What do you think? Which one of the three (the original one + the two above) do you prefer most?

[anthonybrandon]

Sorry about that, the coding style for my current project made its way into this PR.
I like the first option you gave, but I wonder if there is a reason to use the index_sequence?
We could do something along these lines:

    namespace detail
    {
        template <typename _Element, typename ..._Elements>
        bool deserialize_variant(Message& msg, std::variant<_Elements...>& value, const std::string& signature)
        {
            if (signature != signature_of<_Element>::str())
                return false;

            _Element temp;
            msg.enterVariant(signature);
            msg >> temp;
            msg.exitVariant();
            value = std::move(temp);
            return true;
        }
    }

    template <typename ..._Element>
    inline Message& Message::operator>>(std::variant<_Element...>& value)
    {
        std::string type;
        std::string contentType;
        peekType(type, contentType);
        bool result = (detail::deserialize_variant<_Element, _Element...>(*this, value, contentType) || ...);
        SDBUS_THROW_ERROR_IF(!result, "Failed to deserialize variant: signature did not match any of the variant types", EINVAL);
        return *this;
    }

What do you think? Now there is also only one function and both seem pretty simple to me.
The only part I don't like is having to repeat the _Elemen, Element... at the call site.

I also have a commit adding std::variant to the table of supported types. Should I add it to this PR?

[sangelovic]

@anthonybrandon Oh right, of course, we can do the unfolding directly on _Elements..., no need for the index_sequence here, good point! In addition, there is no need to explicitly mention _Elements... template parameter at the call site, because the _Elements... template argument will be deduced automatically from the variant function parameter:

namespace detail
{
    template <typename _Element, typename... _Elements>
    bool deserialize_variant(Message& msg, std::variant<_Elements...>& value, const std::string& signature)
    {
        if (signature != signature_of<_Element>::str())
            return false;

        _Element temp;
        msg.enterVariant(signature);
        msg >> temp;
        msg.exitVariant();
        value = std::move(temp);
        return true;
    }
}

template <typename... Elements>
inline Message& Message::operator>>(std::variant<Elements...>& value)
{
    std::string type;
    std::string contentType;
    peekType(type, contentType);
    bool result = (detail::deserialize_variant<Elements>(*this, value, contentType) || ...);
    SDBUS_THROW_ERROR_IF(!result, "Failed to deserialize variant: signature did not match any of the variant types", EINVAL);
    return *this;
}

With this I think we have a nice, acceptable solution, don't we? Feel free to update the MR, and please can you also add std::variant to the table of supported types... Thanks.

[anthonybrandon]

Okay, right this looks good. I'll update the MR. For some reason I thought you had to specify all template parameters if you specified any.

[sangelovic]

Looks good, thank you, will merge it.

One additional thought... The generated bindings use sdbus::Variant because it's a type erased class, whereas std::variant's internal types form its class type and the binding generator has no idea what these types might be. Would it make sense if we add a converting constructor of sdbus::Variant from std::variant (and, conversely, a conversion operator from sdbus::Variant to std::variant)? It could simplify the interoperability between these two D-Bus variant representations. What do you think?

[anthonybrandon]

I think it would make sense. I can implement it if you like.

[sangelovic]

Of course, looking forward to your PR ;o)

…

-------- Pôvodná správa -------- 1. 4. 2024 13:29, Anthony Brandon napísal/a:

I think it would make sense. I can implement it if you like. — Reply to this email directly, [view it on GitHub](#415 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABM3OUVG62G5UA5IQC6QKBDY3FAIZAVCNFSM6AAAAABEAHXXGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRZGYYTMNZUHE). You are receiving this because you modified the open/close state.Message ID: ***@***.***>