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

Include RapidJSON headers only if BOND_SIMPLE_JSON_PROTOCOL defined #44

Open
oliora opened this issue Mar 9, 2015 · 5 comments
Open

Comments

@oliora
Copy link
Contributor

oliora commented Mar 9, 2015

Correct me if I'm wrong but RapidJSON headers needed only for simple JSON protocol. But this headers are included in any case. This forces a user to has a dependency on RapidJSON even if he/she is not going to use JSON at all.
I propose to include simple_json_xxx headers (along with RapidJSON headers) only if BOND_SIMPLE_JSON_PROTOCOL is defined.

@chwarr
Copy link
Member

chwarr commented Apr 8, 2016

Yes, this makes sense.

@sapek
Copy link
Contributor

sapek commented Apr 19, 2016

The boost::variant used in the implementation of bonded<T> always has all protocols, even disabled ones. The reason for this was to minimize potential for catastrophic, potentially exploitable and very hard to diagnose errors when ODR rule violation would lead to wrong layout of the internal variant data.

@oliora
Copy link
Contributor Author

oliora commented Apr 19, 2016

I've checked the code. The bonded type that minded as protocol agnostic depends on all protocols. That's really strange for me. How this approach will allow to add new protocols without breaking binary compatibility with old code?

@chadwalters
Copy link
Contributor

Revisiting this old issue. Many of the ODR-related concerns and the issue raised above about bonded<> are now addressed as of #425. However, SimpleJSON still appears in the list of BuiltInProtocols. I think that this would be a fairly impactful breaking change for some of the heavier users of Bond internally.

@chwarr
Copy link
Member

chwarr commented Jul 13, 2017

Agreed about the breakage. RapidJSON could be made opt-out pretty easily now, though.

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

No branches or pull requests

4 participants