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

exists old namespace #186

Closed
nori0428 opened this issue Jan 8, 2015 · 6 comments
Closed

exists old namespace #186

nori0428 opened this issue Jan 8, 2015 · 6 comments

Comments

@nori0428
Copy link
Contributor

nori0428 commented Jan 8, 2015

msgpack::type exists in some files.

s/msgpack::type/msgpack::v1::type/g?

  • include/msgpack/adaptor/nil.hpp
msgpack::type::nil v;
  • include/msgpack/adaptor/detail/cpp03_define.hpp
  • include/msgpack/adaptor/detail/cpp11_define.hpp
#define MSGPACK_DEFINE(...) \
    template <typename Packer> \
    void msgpack_pack(Packer& pk) const \
    { \
        msgpack::type::make_define(__VA_ARGS__).msgpack_pack(pk); \
    } \
    void msgpack_unpack(msgpack::object const& o) \
    { \
        msgpack::type::make_define(__VA_ARGS__).msgpack_unpack(o); \
    }\
    template <typename MSGPACK_OBJECT> \
    void msgpack_object(MSGPACK_OBJECT* o, msgpack::zone& z) const \
    { \
        msgpack::type::make_define(__VA_ARGS__).msgpack_object(o, z); \
    }
  • include/msgpack/adaptor/detail/cpp03_msgpack_tuple.hpp
    But commented out.any intended?
@nori0428
Copy link
Contributor Author

nori0428 commented Jan 8, 2015

BTW,
Thanks to fix #149 and related to.;)

@redboltz
Copy link
Contributor

redboltz commented Jan 8, 2015

I think that they should be type::nil and type::make_define...
Because the types are part of the v1 implementation. If it's not, it should be msgpack::type::some_types.
Minimizing explicit versioning makes code maintenance easier.

msgpack::type::tuple should be type::tuple. The reason those codes are commented out, the original codes are commented out. I don't know why, but I just keep the original code.

@nori0428
Copy link
Contributor Author

nori0428 commented Jan 8, 2015

Thanks for rapid reply.
I agree about type::nil and understood about msgpack::type::tuple.

But MSGPACK_DEFINE is a MACRO.I think that it's difficult to use type::make_define.
shown in the following.

#include "msgpack/msgpack.hpp"

namespace MyApp {

struct Foo {
  int a;

  MSGPACK_DEFINE(a);
}

} // namespace MyApp

Do we have to use MSGPACK_API_VERSION_NAMESPACE when using MSGPACK_DEFINE?

@nori0428
Copy link
Contributor Author

nori0428 commented Jan 8, 2015

Ah, sorry.
I‘m something wrong... example/class_instrusive.cpp works fine.
I check my code.

@redboltz
Copy link
Contributor

redboltz commented Jan 8, 2015

Ah, I remembered my decision.
Because of MACRO, MSGPACK_DEFINE is NOT a part of v1 interface. It is a common interface. So it uses msgpack::type::* intentionally. When clients uses MSGPACK_DEFINE, default or explicitly selected version of make_define is used.

I think that it is a reasonable design choice.

Do we have to use MSGPACK_API_VERSION_NAMESPACE when using MSGPACK_DEFINE?
No.

See:

#include <sstream>
#include <msgpack.hpp>

struct S {
    int i;
    MSGPACK_DEFINE(i);
};

int main() {
    std::stringstream ss;
    S s;
    msgpack::pack(ss, s);
    assert(msgpack::unpack(ss.str().data(), ss.str().size()).get().as<S>().i == s.i);
}

@nori0428
Copy link
Contributor Author

nori0428 commented Jan 8, 2015

Thanks, this was my mistake.I'm sorry for troubling you.

@nori0428 nori0428 closed this as completed Jan 8, 2015
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