ecorm/cppwamp

Unused serialization libraries still needed to build

taion opened this issue · 12 comments

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:

switch (codecId)
{
case CodecId::json:
return Client<Json, Transport>::create(std::move(trn));
case CodecId::msgpack:
return Client<Msgpack, Transport>::create(std::move(trn));
default:
assert(false && "Unexpected CodecId");
}

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?

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.

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.

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++.

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!

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?

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/

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.

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

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.

something like TcpConnector<Msgpack>?

Yep!

The new API is really cool. 👍

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.