Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compile-time check if adaptor for type exists #1051

Open
timblechmann opened this issue Feb 6, 2023 · 8 comments
Open

compile-time check if adaptor for type exists #1051

timblechmann opened this issue Feb 6, 2023 · 8 comments

Comments

@timblechmann
Copy link

more a question than a bug, but something that i've been wondering without having found a good answer, yet:

i'd like to detect at compile time via a constexpr function if an adaptor for a specific type exists:

template <typename T>
constexpr bool msgpack_can_pack();

currently we have:

template <typename T, typename Enabler>
struct convert {
    msgpack::object const& operator()(msgpack::object const& o, T& v) const;
};

operator() is either implemented by a specialization or it is using the default implementation at:

template <typename T, typename Enabler>
inline
msgpack::object const&
adaptor::convert<T, Enabler>::operator()(msgpack::object const& o, T& v) const {
    v.msgpack_unpack(o.convert());
    return o;
}

so afaict we may need two answer two question:

  • can we use enable_if to check if the operator() can be implemented for a specific type?
  • can we check if operator() exists in order to implement msgpack_can_pack?

thoughts?


i've done a few experiments:

diff --git a/include/msgpack/v1/adaptor/adaptor_base.hpp b/include/msgpack/v1/adaptor/adaptor_base.hpp
index 489362b..b2291ec 100644
--- a/include/msgpack/v1/adaptor/adaptor_base.hpp
+++ b/include/msgpack/v1/adaptor/adaptor_base.hpp
@@ -18,17 +18,43 @@ namespace msgpack {
 MSGPACK_API_VERSION_NAMESPACE(v1) {
 /// @endcond
 
 
 namespace adaptor {
 
+namespace impl {
+
+template < typename T >
+using msgpack_unpack_t
+    = decltype( std::declval< T >().msgpack_unpack( std::declval< msgpack::object::implicit_type >() ) );
+
+template < typename T, typename = std::void_t<> >
+struct has_msgpack_unpack : std::false_type
+{};
+
+template < typename T >
+struct has_msgpack_unpack< T, std::void_t< msgpack_unpack_t< T > > > : std::true_type
+{};
+
+template < typename T >
+inline constexpr bool has_msgpack_unpack_v = has_msgpack_unpack< T >::value;
+
+} // namespace impl
+
+
 // Adaptor functors
 
-template <typename T, typename Enabler>
-struct convert {
-    msgpack::object const& operator()(msgpack::object const& o, T& v) const;
+template<typename T, typename Enabler>
+struct convert
+{
+};
+
+template<typename T>
+struct convert<T, std::enable_if_t<impl::has_msgpack_unpack_v<T>>>
+{
+    msgpack::object const &operator()(msgpack::object const &o, T &v) const;
 };
 
 template <typename T, typename Enabler>
 struct pack {
     template <typename Stream>
     msgpack::packer<Stream>& operator()(msgpack::packer<Stream>& o, T const& v) const;
diff --git a/include/msgpack/v1/object.hpp b/include/msgpack/v1/object.hpp
index fda54be..a9ac15a 100644
--- a/include/msgpack/v1/object.hpp
+++ b/include/msgpack/v1/object.hpp
@@ -637,16 +637,16 @@ struct packer_serializer {
         return o;
     }
 };
 } // namespace detail
 
 // Adaptor functors' member functions definitions.
-template <typename T, typename Enabler>
+template <typename T>
 inline
 msgpack::object const&
-adaptor::convert<T, Enabler>::operator()(msgpack::object const& o, T& v) const {
+adaptor::convert<T, std::enable_if_t<adaptor::impl::has_msgpack_unpack_v<T>>>::operator()(msgpack::object const& o, T& v) const {
     v.msgpack_unpack(o.convert());
     return o;
 }
 
 template <typename T, typename Enabler>
 template <typename Stream>

this would allow me to write something like:

template < typename T, typename = void >
struct has_msgpack_convert_helper : std::false_type
{};

template < typename T >
struct has_msgpack_convert_helper< T,
                                   std::void_t< decltype( std::declval< msgpack::v3::adaptor::convert< T > >()(
                                       std::declval< msgpack::object const& >(), std::declval< T& >() ) ) > >
    : std::true_type
{};

template < typename T >
constexpr bool has_msgpack_convert = has_msgpack_convert_helper< T >::value;

any thoughts on such functionality in the library?

@redboltz
Copy link
Contributor

redboltz commented Feb 6, 2023

Here is a small example to check adaptor support.
It checks intrusive pack adaptor only. Maybe there is some way to check non-intrusive adaptor. But I'm not sure.
I use C++17 experimental feature. Maybe there would be a better way but I coulldn't found. I'm not expert of this kinds of programming.

#include <msgpack.hpp>
#include <iostream>
#include <experimental/type_traits>

struct foo {};
struct bar {
    int i;
    MSGPACK_DEFINE(i);
};


template <typename T, typename Stream>
using msgpack_pack_func = decltype(std::declval<T>().msgpack_pack(std::declval<msgpack::packer<Stream>&>()));

template <typename T, typename Stream>
using has_msgpack_pack = std::experimental::is_detected<msgpack_pack_func, T, Stream>;

// check only intrusive pack adaptor
// maybe there is some way to check non-intrusive adaptor
template <typename T, typename Stream>
void my_pack(Stream& buf, T t) {
    if constexpr(
        has_msgpack_pack<T, Stream>::value // || non-intrusive checker
    ) {
        msgpack::pack(buf, t);
        std::cout << "match" << std::endl;
    }
    else {
        std::cout << "not match" << std::endl;
    }
}

int main() {
    msgpack::sbuffer buf;
    my_pack(buf, foo{});
    my_pack(buf, bar{});
    my_pack(buf, 1); // not match because only intrusive adaptor check
}

@timblechmann
Copy link
Author

yes, the non-intrusive check is the challenging part.

though i wonder if you have any thoughts about the approach that i've outlined above: my approach is intrusive into the convert base class, but that way it will not get a compile error that msgpack_pack/msgpack_unpack does not exist for a type, but it would provide operator()(msgpack::object const& o, T& v) only if that function exist. that would allow us to use SFINAE to check of the operator() compiles

@redboltz
Copy link
Contributor

redboltz commented Feb 6, 2023

Unfortunately, I don't have good idea. The current msgpack-c SFINAE is already been complecated due to support multiple versions of API (v1...v3). I don't want to modify the current SFINAE mechanism.
If the checking can be achieved like my example, it is very good. So trying non-intrusive version without msgpack-c library modification is good.

@timblechmann
Copy link
Author

If the checking can be achieved like my example, it is very good. So trying non-intrusive version without msgpack-c library modification is good.

unfortunately i don't see any way to achieve this as there is no way for the compiler do distinguish between "template<typename T, typename Enabler> struct convert is instantiated" and "template<> struct convert<some_type> is specialised". one could to identify some specific convert specialisations (e.g. by using custom members/tags, which is my current workaround).

@redboltz
Copy link
Contributor

redboltz commented Feb 7, 2023

Ok, I unsderstand you already been tried it and #1051 (comment) describes it.
I guess that the key feature is expression SFINAE that has been introduced since C++11.
Maybe the following part of your code.

+template < typename T >
+using msgpack_unpack_t
+    = decltype( std::declval< T >().msgpack_unpack( std::declval< msgpack::object::implicit_type >() ) );

msgpack-c need to support C++03. So C++11 code in the library need to be added very carefully.
Even if https://github.com/msgpack/msgpack-c/wiki/v2_0_cpp_configure#msgpack_use_cpp03 is defined, the client code need to compile and work successfully. If that are satisfied, I would accept the code modification to support compile time checking the functionality existance. I guess that it is for C++11 and later.

@timblechmann
Copy link
Author

let me try to flesh this out in code and we can discuss in a PR.

out of curiosity: i wonder what's the reason of using c++03 as minimum requirement in 2023? (e.g. c++11 is around for 10+ years already)

@redboltz
Copy link
Contributor

redboltz commented Feb 7, 2023

out of curiosity: i wonder what's the reason of using c++03 as minimum requirement in 2023? (e.g. c++11 is around for 10+ years already)

When I inherit msgpack-c project, C++03 was widely used. Now, I personally don't think so. I personally want to migrate at least C++17. But removing support is very difficult.

In my experience at msgpack-c project, I did

  1. Separated C and C++ projects using branch. Other approach is separate directory like msgpack-c/c msgpack-c/c++, or separate repository (existing PRs and issues contain C and C++ at that time). We evaluated many pros/cons and finally chose branch. But I still receive nagative reaction ;).
  2. Replace hidden dependency of the Boost with explicit. Similarlly I receive negative reaction.

Even if they are not removing functionality, basically.

I guess removing C++03 support is harder.
I think that if I really want to do that, I create a new repository something like msgpack-modern-c++ and minimum compiler requirement C++17 or 20 :)
I don't have any actual plan to do that. I have higher priority projects, so far.

@timblechmann
Copy link
Author

I guess removing C++03 support is harder.

i guess it would be entirely appropriate to kindly ask people who cannot adopt c++11 in 2023 to use an older released version of the library. that said, in 2015 it was still controversial to require c++11 as some organisations use toolchains that are a few years old (just like using c++20 in 2023 is still a bit controversial).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants