-
Notifications
You must be signed in to change notification settings - Fork 219
Use cpp17 and no need for Boost if you disable examples. #284
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
Conversation
...se it was useless actually.
I replaced the boost::optional and boost::any with C++17 ones (project is set to use C++17). However, examples still use boost and I have no idea yet how to refactor that. Why I did it ? To save space if someone just wants to install the library.
Edit: if you don't want to break compatibility, I am planning to add which C++ to use for example so that it would be available in old way or new one ?
Given this is quite the breaking change, it should be an optional feature that we can eventually enable by default in say version 1.0. Using std::optional is nice but it will break any existing user's codes, plus it involves them using a newer standard (which is reasonable given C++17 is 4 years old, but still).
Could you turn this into a configurable option? e.g. cmake -DUSE_STD_OPTIONAL and have some header that defines cppkafka::optional to be std::optional or boost::optional depending on it and using cppkafka::optional internally rather than the concrete optional type we're using?
Sure then I will do it and let you know)) Thanks.
8b01d1a to
6213cd3
Compare
I did it and if someone wants to use only C++17 without any boost, they should do this:
cmake -DUSE_CPP17=ON -DCPPKAFKA_DISABLE_EXAMPLES=ON -DCPPKAFKA_DISABLE_TESTS=ON "DIR"
Also, I fixed some conflicts with max and min on windows so I fixed them. @mfontanini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is enough. There needs to be a record somewhere (e.g. in a generated header file) that stores this and code should include that file to figure out this flag.
If you don't do that, you can have the library being compiled with this flag on but an application that uses the library not knowing (for obvious reasons) that they need to set this macro manually, which will break badly because from one's perspective you're using std types and from the other's boost ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be set as a definition at the cmake level if it's needed, rather than here in this particular file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume both optionals have operator* so rather than doing this you can maybe use that which should make the same expression work for both types?
I replaced the boost::optional and boost::any with C++17 ones (project is set to use C++17). However, examples still use boost and I have no idea yet how to refactor that. Why I did it ? To save space if someone just wants to install the library.