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

Switch to libb64 for base64 encoding/decoding. #1046

Merged
merged 3 commits into from
Jul 18, 2017

Conversation

de-vri-es
Copy link
Contributor

libxmlrpc++ contains an unlicensed base64 implementation (see also #1034). This PR replaces it with libb64, a public domain base64 implementation [1]. Benchmarks also indicate that it is significantly faster than the old implementation for large input (more details at #1034).

[1] http://libb64.sourceforge.net/

@tfoote
Copy link
Member

tfoote commented May 8, 2017

@de-vri-es Thanks for taking the time to track this down and the study of alternatives and put together this replacement.

@dirk-thomas
Copy link
Member

I am sorry for the late response. The patch looks good to me. The new test could use the ROS code style (rather than tabs etc.) but that is not an important thing.

Since the public header xmlrpcpp/base64.h this change might break downstream packages. As far as I see it that it intentional. To make it easier for users which run into this I propose to add a new file with that name which contains just a compile time error message (e.g. #error some text to explain what happened and how to resolve it).

@de-vri-es
Copy link
Contributor Author

I updated the test to follow ROS style (atleast, spaces and curly braces on new lines). Let me know if I should do more on it.

Since the public header xmlrpcpp/base64.h this change might break downstream packages. As far as I see it that it intentional. To make it easier for users which run into this I propose to add a new file with that name which contains just a compile time error message (e.g. #error some text to explain what happened and how to resolve it).

Indeed, I considered the base64 encoding to be an internal detail. No need to expose it as a public header. Keeping it internal frees us from having to provide API stability. And if people want to do base64 encoding they're better off finding a dedicated library for that than piggybacking on xmlrpc.

But you're right that the old header was public, so an #error makes sense. I added one with a reference to this pull request. It can probably be removed again in ROS M or N.

@dirk-thomas
Copy link
Member

Thank you for the quick update!

@dirk-thomas dirk-thomas merged commit 7019fb1 into ros:lunar-devel Jul 18, 2017
sputnick1124 pushed a commit to sputnick1124/ros_comm that referenced this pull request Jul 30, 2017
* Replace base64.h with libb64.

* Add unit test for base64 encoding/decoding.

* Add a base64.h with #error explaining why it is removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants