msgpack / msgpack-c

MessagePack implementation for C and C++ / msgpack.org[C/C++]
Other
3.03k stars 883 forks source link

`std::variant` support #1074

Closed uyha closed 1 year ago

uyha commented 1 year ago

I would like to ask about the support for std::variant, is there any plan on supporting it? Does it require some help?

redboltz commented 1 year ago

I would like to ask about the support for std::variant, is there any plan on supporting it?

No. See #462. It is about boost variant, but the essense of the issue is the same. MessagePack types sometimes can be mapped multiple variant types, but user often has expected mapping. So some meta data is required but what kind of meta data is good? It depends on user's usecase. The msgpack-c library shouldn't enforce a format and mapping of the meta data.

Users can define their own adaptor using https://github.com/msgpack/msgpack-c/wiki/v2_0_cpp_adaptor#non-intrusive-approach

uyha commented 1 year ago

I agree that the library should not enforce the metadata, but providing a sane default is still reasonable (gated behind some macro guard). As for what a sane default should look like, I think we should focus on the case that both the serializing and deserializing sides have the same definition of the std::variant being transfered. Since std::variant provides the index member function, it should be encoded in the msgpack structure. After the index, then it's the actual msgpack structure for the actual value.

For example, if we have two classes A and B, and they both provide the appropriate functions for serializing and deserializing, then std::variant<A, B> should be encoded as an array with 2 elements (first byte being 0b10010010). The 1st element shall be the integer returned by the std::variant::index function, and the 2nd element shall be retrieved by calling the appropriate de/serialize functions.

One case should be taken care of is the valueless case (valueless_by_exception), in this case either throwing or doing nothing is reasonable.

redboltz commented 1 year ago

Ok, I understand. But some of users would already be defined a costom adaptor. So as you mentioned, some cacro guard should be defined. I think that MSGPACK_USE_STD_VARIANT_ADAPTOR is good.

#if defined(MSGPACK_USE_STD_VARIANT_ADAPTOR)

// `[index, T]` based std::variant adaptor here

#endif // defined(MSGPACK_USE_STD_VARIANT_ADAPTOR)

Could you create a PR for that?

std::variant is introduced since C++17. So you can refer to https://github.com/msgpack/msgpack-c/blob/cpp_master/include/msgpack/v1/adaptor/cpp17/optional.hpp Some more implementation at other part of the code is requred including tests.

uyha commented 1 year ago

Thank you for being open to accepting a PR. I will create one later.

redboltz commented 1 year ago

NOTE:

For CI, you would need to modify the following part for testing:

https://github.com/msgpack/msgpack-c/blob/cpp_master/.github/workflows/coverage.yml#L51 https://github.com/msgpack/msgpack-c/blob/cpp_master/.github/workflows/gha.yml#L147 https://github.com/msgpack/msgpack-c/blob/cpp_master/ci/build_cmake.sh#L16 https://github.com/msgpack/msgpack-c/blob/cpp_master/CMakeLists.txt#L28

MSGPACK_USE_STD_VARIANT_ADAPTOR is not only C++ preprosessor macro but also cmake option like MSGPACK_USE_X3_PARSE.

uyha commented 1 year ago

implemented in #1075

iamOgunyinka commented 10 months ago

Hi @uyha, could you (or @redboltz) please give an example usage of your support for std::variant<T, ...>? I have compiled msgpack-c with -DMSGPACK_USE_STD_VARIANT_ADAPTOR=ON and MSGPACK_DEFINEd the types used in this variant but the call to convert wouldn't work for me. A simple example below.

struct Foo {
  std::string a;
  int b = 0;
  MSGPACK_DEFINE(a, b);
};

struct Bar {
  double c {};
  MSGPACK_DEFINE(c);
};

using foobar_variant_t = std::variant<Foo, Bar>;

int main() {
  msgpack::sbuffer outBuffer;
  msgpack::pack(outBuffer, Bar{1.0});

  auto const unpacked_data = msgpack::unpack(outBuffer.data(), outBuffer.size());
  auto object = oh.get();

  Bar temp {};
  object.convert(temp);  // this line doesn't work for me
  return 0;
}

Is there anything specific I need to do to make this work?

redboltz commented 10 months ago

@iamOgunyinka

#include <cassert>
#include <msgpack.hpp>

struct Foo {
  std::string a;
  int b = 0;
  MSGPACK_DEFINE(a, b);
};

struct Bar {
  double c {};
  MSGPACK_DEFINE(c);
};

using foobar_variant_t = std::variant<Foo, Bar>;

int main() {
  msgpack::sbuffer outBuffer;
  foobar_variant_t v1{Bar{1.0}};
  msgpack::pack(outBuffer, v1);

  auto oh = msgpack::unpack(outBuffer.data(), outBuffer.size());
  auto object = oh.get();

  auto v2 = object.as<foobar_variant_t>();
  auto bar = std::get<Bar>(v2);
  assert(bar.c == 1.0);
  return 0;
}