Include ordering results in compilation errors #249
Comments
|
Unfortunately, msgpack-c requires correct including order. It is because in order to achieve API versioning. See https://github.com/msgpack/msgpack-c/wiki/cpp_versioning and #135 https://github.com/msgpack/msgpack-c/wiki/cpp_packer#packing-enums msgpack-c used to depends on ADL, thanks to two phase name lookup caused by ADL, the overloding functions' point of instanciation is delayed. However, ADL cannot work well with API versioning. Because API versioning requires namespace qualification. We've made the decision that API-versioning should be added even though including order limitation is introduced. As I wrote above, I prepared some documents on wiki and examples. But the documents wrote only about the order of msgpack_fwd.hpp and msgpack.hpp. Because many users only includes that two files. In your case, you include object.hpp directly. I think that I should write more about include files in the documentation. |
|
I think I have another problem with this. I have a project that includes msgpack.hpp in many different header files. The problem is that as soon as I have any inline code in my common header files that needs msgpack.hpp rather than msgpack_fwd.hpp it forces me to have to include msgpack.hpp in a header file. On a larger project where include chains are deep this becomes very difficult to manage. What is the recommended strategy for including msgpack_fwd.hpp and msgpack.hpp? Does this mean that I am now forced to not inline code in these cases? I have another third-party header only library that depends on mspack-c. So now I am forced to include msgpack.hpp in header files. So now I have to worry about these things getting included in the right order so that one of them doesn't introduce a msgpack.hpp include too early? |
|
I understand your situation. I considered that how to solve the issue. I've sent the PR #261 to minimize dependency to object.hpp. I believe that a header only library that uses msgpack-c need to include only msgpack_fwd.hpp when the PR is applied. Could you get the branch and test it? After I've created the PR, I've noticed that I had ought to name as follows: msgpack_fwd.hpp -> msgpack.hpp However, it's too late... Could you create workaround directory and set it to earlier include search path as folows? Place the following files in workaround directory: workaround/msgpack.hpp Each file has the following contents: workaround/msgpack.hpp #ifndef WORKAROUND_MSGPACK_HPP
#define WORKAROUND_MSGPACK_HPP
#include "path_to_msgpack/include/msgpack_fwd.hpp"
#endifworkaround/msgpack_impl.hpp #ifndef WORKAROUND_MSGPACK_IMPL_HPP
#define WORKAROUND_MSGPACK_IMPL_HPP
#include "path_to_msgpack/include/msgpack.hpp"
#endif[NOTE: left path has higher priority] msgpack-c never include msgpack.hpp internally. So this workaround should work. Now, the library header files don't include path_to_msgpack/include/msgpack.hpp. Finally, include workaround/msgpack_impl.hpp from your_program.cpp after other headers inclusion. your_program.cpp #include <third_party_header1.hpp>
#include <third_party_header2.hpp>
#include <your_header1.hpp>
#include <your_header2.hpp>
// ...
#include <msgpack_impl.hpp>MechanismThe template versions of packing/converting operators are defined in object.hpp. In order to dispatch to the apropriate overload of the packing/converting operator, all overloads should be declared before invoking them. So I created *_fwd.hpp files and include them from object.hpp. It works well. To support user defined types, I created msgpack_fwd.hpp. It originally contains a minimal information to declare the operators for user defined types. But now, msgpack_fwd.hpp has a most of msgpack function except object.hpp. |
|
I wrote a workaround approach above. Here is a straight forward approach. After applying #261, libraries that use msgpack-c replace msgpack.hpp with msgpack_fwd.hpp when the libraries support msgpack-c 1.0.0 or later. Both workaround approach and straight forward approach work fine in my small example. I believe that it should work well on your project. Please let me know your result. |
|
I seem to still have a problem with cases where msgpack::type::nil is used within header files: |
|
Thank you for reporting! I will check it soon. |
|
Looks like moving object::with_zone into object_fwd.hpp is the right thing to do. |
|
I am also finding that I need to now include msgpack/adaptor/nil.hpp into header files. Is this ok? |
|
I've just committed redboltz@a35acc6 |
|
Ok cool. I also see that unpack.hpp includes object.hpp. So if I declare a msgpack::unpacker as a class member variable I am now pulling in object.hpp. |
It has been fixed in #261 |
|
I almost feel like all of this could have been avoided if we had taken an approach similar to std::hash. For example, if you want to define a hash operator for a custom type: We could have taken the very same approach with msgpack-c and defined operator<< and operator>> in the same manner. Then we wouldn't have all of these include ordering constraints. |
Yes, it is. Or you can include msgpack/type.hpp. |
It seems to be excellent idea!! msgpack-c 0.5.9 and older version uses ADL and function (operator<< and >> ) overloading in order to appropriate dispatch. It cannot work well with API versioning. So I introduced include ordering approach. It solves both appropriate dispatch and API versioning. However, include ordering is not easy to use. I thought that we need to accept that inconvenience. http://melpon.org/wandbox/permlink/Xc5K9CuFPsZzCEhO Using function template (operator<< and >>) specialization instead of overloading solves both appropriate dispath and API versioning. And there is no include ordering restriction. http://melpon.org/wandbox/permlink/sTVu0SAHTTXbPtUJ I will apply your approach to msgpack-c. It could be a pretty big update. I need time to implement and test them. But I will do it as soon as possible. Thank you @davidchappelle, I appreciate your help. |
|
That is awesome. Let me know if there is anything I can do to help test. |
|
Thanks. I meet with a problem. Packing function templates cannot be specialized. They already have a template parameter Stream. Stream should be kept as the template parameter. So I need function template partial specialization. template <typename Stream>
inline msgpack::packer<Stream>& operator<< <UserType> (msgpack::packer<Stream>& o, const UserType& v) {
...
}However, C++ cannot allow the function template partial specialization. Here is a proof of concept codes: When the first parameter of operator<< is not a template, it works as expected: When I try to partial specialization, it fails to compile. See line 94 When I use function template overloads instead of partial specialization, user defined version is not called. When I back to the include ordering approach, user defined version is called as expected. See line 34. Hmm... I couldn't find a good solution, so far. |
|
I think that using a class template instead of a function template is basic approach to solve these kinds of C++ restriction. Packing/Converting functions could be a functors. At least they don't need to be operators. I think that it is a good approach for the future. But it could break existing libraries that use msgpack-c if they define user defined Packing/Converting functions. |
|
Here is a solution I think. |
|
The functor approach you posted above is pretty much what I was originally thinking. However, it seems that no matter which path we take we will introduce a breakage in existing libraries. if we are going to break it then I would prefer to take the functor approach although I believe that the solution I posted above will also work. |
|
http://melpon.org/wandbox/permlink/PJFfqdbPAcIhcrtl Expected output is Because user defined version that outputs start with "OL" should be called. To create perma link on wandbox, |
|
Ok. So I think that the only real viable solution here is going to be to use functor approach. |
|
I prefer to functor approach too. I will replace adaptors implementation from operators(functions) to functors. At first, you have pointed out two problems.
I think that the first problem is more important. I will create the new developping branch to implement functor approach. I will discuss with @nobu-k, another maintainer, which version should we include the functor approach. |
|
Thank you for your help on this issue. Let me know how things progress and if I can help out in any way. |
|
I've just finished implementing the functor based approach. It is #262. |
|
Just on my way in to Toronto. I'll be able to start checking it in a few hours. |
|
Tested agaisnt msgpack-rpc-boost and it worked perfectly fine. Now testing against a larger project that uses more of the custom stream serialization/de-serialization. |
|
Integrated very well with the larger project. |
|
Thank you for testing! I am writing the document about this new approach. https://github.com/redboltz/msgpack-c/wiki |
|
msgpack-c 1.1.0 has been released just now. The issue is fixed. @davidchappelle, thank you for your help. I thought that API versioning and avoiding include ordering are trade off. But it is not true. Both goals are has been archieved now. I believe that it is one of a great msgpack-c improvement. |
Here is a small sample program that shows how include ordering can result in a significant amount of compiler errors. Forcing a strict ordering on includes seems to be a very poor approach:
If you compile this program you will get the following output. Swapping out the msgpack/object.hpp includes will get rid of the errors.
The text was updated successfully, but these errors were encountered: