ecorm / cppwamp

C++ client library for the WAMP protocol.
Boost Software License 1.0
35 stars 9 forks source link

Unused serialization libraries still needed to build #64

Closed taion closed 9 years ago

taion commented 9 years ago

I'm trying to build a msgpack-only application, but I get build errors from not having RapidJson installed.

I believe it's from here: https://github.com/ecorm/cppwamp/blob/master/cppwamp/include/cppwamp/internal/client.hpp#L23-L24 because client codec is resolved as so: https://github.com/ecorm/cppwamp/blob/6defeb0bbbb4b99c3f0d374618b3dacd2c861fb0/cppwamp/include/cppwamp/internal/client.hpp#L783-L793

I'd prefer not to depend on RapidJson when I'm not using JSON serialization.

ConnectorList destroys all static type information on the Connectors holding the CodecIds, so what do you think of making CodecId slightly fatter, perhaps with a virtual method for creating the correct client?

ecorm commented 9 years ago

When using the library in a header-only fashion, you should not be required to have RapidJSON installed if you only intend to use MsgPack. I'm therefore labeling this as a bug.

ConnectorList destroys all static type information on the Connectors holding the CodecIds, so what do you think of making CodecId slightly fatter, perhaps with a virtual method for creating the correct client?

That would work, but would introduce a dependency from Codec<Id> to Client. I have plans on reusing the Variant + Codec stuff in other parts of my app that have nothing to do with WAMP.

I'll try to think of a solution that fixes this bug without adding said dependency.

ecorm commented 9 years ago

One possibility is to make TcpConnector and UdsConnector class templates that take the desired Codec as a template parameter. The Connector subclasses could then directly create the appropriate Client<Codec, Transport> object here, which would eliminate the need for the createClient factory function.

The problem with this approach, is that turning TcpConnector and UdsConnector into class templates would break the Pimpl compiler firewall.

ecorm commented 9 years ago

ConnectorList destroys all static type information on the Connectors holding the CodecIds, so what do you think of making CodecId slightly fatter, perhaps with a virtual method for creating the correct client?

Oh, now I get what you meant by this. I think you meant that instead of passing integer IDs, users would pass lightweight Client<Codec, Transport> factories. I'll have to think about that some more...

... The virtual factory method idea can't work because of the Transport template parameter of Client<Codec, Transport>. You can't have virtual template methods in C++.

ecorm commented 9 years ago

Yay, I've figured it out! I can have templated TcpConnector/UdsConnector classes and still be able to use the Pimpl idiom. The trick is use explicit instantiation when the compiled version of the library is being used.

I was never fond of passing Codec IDs to the connectors and would have preferred a compile-time dependency-injection approach. With explicit instantiation, I can go back to dependency injection and still be able to use Pimpl in the compiled version of the library!

taion commented 9 years ago

I realized the virtual template thing shortly after my flight took off. Oops. This is complicated, eh?

I don't fully follow, though - how does ConnectorList work if TcpConnector and UdsConnector are templated? Or is the idea that ConnectorList should become something different?

ecorm commented 9 years ago

Template classes can inherit from a non-template base class. Remember that ConnectorList is simply a vector of Connector shared pointers, and that Connector is an abstract base class. Even though TcpConnector<TCodec> and UdsConnector<TCodec> are template classes, they can still inherit from Connector.

Connector would end up implementing type erasure , and would hide the TCodec template parameters.

BTW, type erasure is the technique used behind boost::any: https://akrzemi1.wordpress.com/2014/01/13/type-erasure-part-iv/

taion commented 9 years ago

I see, thanks! I also managed to misread how ConnectorList was defined... not sure how I thought it was going to work otherwise.

Are you planning on fixing this for the next release? If so I'm happy to just work around this for the moment.

ecorm commented 9 years ago

I've added this to the 0.4 milestone, which will follow the 0.3.1 release.

taion commented 9 years ago

Okay, it sounds like you're considering an actual API change for this (something like TcpConnector<Msgpack>?), so I'll just use a workaround for now.

ecorm commented 9 years ago

something like TcpConnector<Msgpack>?

Yep!

taion commented 9 years ago

The new API is really cool. :+1:

ecorm commented 9 years ago

Thanks! I realized that TcpConnector and friends only had one method that was useful for the user, so I replaced them with free factory functions. The addition of the TcpHost and UdsPath objects ended up making the implementation of TcpConnector identical to UdsConnector (and their legacy variants). I was therefore able to consolidate the 4 different Connector subtypes into a single RawsockConnector template.