Low Hanging Fruit: Factor our Base64 Encoding/Decoding into boost/network/utils/ #287

Open
deanberris opened this Issue Aug 20, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@deanberris
Member

deanberris commented Aug 20, 2013

I'm filing this issue so that someone can pick it up. There's code in boost/network/protocol/http/message{hpp.ipp} that does Base64 encoding/decoding. Currently, its UNUSED anywhere in the codebase and making it easier to use and available stand-alone can be done as a simple project.

My suggestion is to move this into separate headers and implementation files (don't need to be header-only), and follow STL algorithm conventions. A good place to put this is in boost/network/utils/ beside the thread_pool.hpp implementation.

@prantlf

This comment has been minimized.

Show comment
Hide comment
@prantlf

prantlf Sep 15, 2013

I followed your suggestion about the implementation and placing. (I hope, at least :-) I believe that a generic BASE64 encoder should support chunked and streamed encoding, which means maintaining an encoding state between the function calls. I updated the gist with the interface to the initial version I'd like to implement.

I tried a couple of approaches how to allow encoding of a chunked input: buffering, introducing state to the boost transforming iterators, or full implementation (like in message.ipp or libbase64). After my initial joy that boost has a generic bit-transforming iterator, I got disappointed, because I wasn't able to introduce the encoding state without modifying the iterators, which basically meant to re-implement them. (Only buffering wouldn't need this.) I checked in the experiments as a benchmark and included the results.

I checked in the initial implementation to the base64_util branch. So far it includes iterator and stream manipulator interface and minimum tests. It satisfies my current use case - Basic Authentication response. I tested it on Ubuntu with GCC 4.6.4 only.

prantlf commented Sep 15, 2013

I followed your suggestion about the implementation and placing. (I hope, at least :-) I believe that a generic BASE64 encoder should support chunked and streamed encoding, which means maintaining an encoding state between the function calls. I updated the gist with the interface to the initial version I'd like to implement.

I tried a couple of approaches how to allow encoding of a chunked input: buffering, introducing state to the boost transforming iterators, or full implementation (like in message.ipp or libbase64). After my initial joy that boost has a generic bit-transforming iterator, I got disappointed, because I wasn't able to introduce the encoding state without modifying the iterators, which basically meant to re-implement them. (Only buffering wouldn't need this.) I checked in the experiments as a benchmark and included the results.

I checked in the initial implementation to the base64_util branch. So far it includes iterator and stream manipulator interface and minimum tests. It satisfies my current use case - Basic Authentication response. I tested it on Ubuntu with GCC 4.6.4 only.

@prantlf

This comment has been minimized.

Show comment
Hide comment
@prantlf

prantlf Sep 15, 2013

Outstanding tasks:

  • add encoding options to support RFC 4648
  • for the stream interface, try transforming streambuf, wrapper stream and a codecvt how they would feel in the user code; the stream manipulators are not convenient for every scenario
  • decide what encoding code to use in the final version - basically, it means implementing the lowest-level encode method which is called by the others and the encode_rest method; the current implemetation is not based on the boost transforming iterators
  • better structure the tests and add more of them
  • add documentation
  • start with the decoder

prantlf commented Sep 15, 2013

Outstanding tasks:

  • add encoding options to support RFC 4648
  • for the stream interface, try transforming streambuf, wrapper stream and a codecvt how they would feel in the user code; the stream manipulators are not convenient for every scenario
  • decide what encoding code to use in the final version - basically, it means implementing the lowest-level encode method which is called by the others and the encode_rest method; the current implemetation is not based on the boost transforming iterators
  • better structure the tests and add more of them
  • add documentation
  • start with the decoder
@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Sep 15, 2013

Member

Awesome stuff @prantlf -- are you ready to send me a PR to get these things moving sooner than later? I'm happy to merge incomplete work into 0.10-devel especially since I've been doing the same anyway. 😀

Member

deanberris commented Sep 15, 2013

Awesome stuff @prantlf -- are you ready to send me a PR to get these things moving sooner than later? I'm happy to merge incomplete work into 0.10-devel especially since I've been doing the same anyway. 😀

@prantlf

This comment has been minimized.

Show comment
Hide comment
@prantlf

prantlf Sep 15, 2013

Thanks for encouraging me! You discovered it damn' quick here :-) I was holding back with the PR not to overwhelm you with additional PRs for changes :-) Initially I liked the function-only interface you have for uri::encode, but I'm not sure if we don't switch switch to a class with static methods, when the encoding options are introduced. I feel that the interface may not be stable yet. Would you like the PR nevertheless?

prantlf commented Sep 15, 2013

Thanks for encouraging me! You discovered it damn' quick here :-) I was holding back with the PR not to overwhelm you with additional PRs for changes :-) Initially I liked the function-only interface you have for uri::encode, but I'm not sure if we don't switch switch to a class with static methods, when the encoding options are introduced. I feel that the interface may not be stable yet. Would you like the PR nevertheless?

@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Sep 15, 2013

Member

Yes -- it would be good if I can review the PR so you get feedback sooner on actual code. I'd be happy to see what you've done in a PR, then when we're happy with the implementation/API we can merge accordingly. Shipping early and often is a good thing. :)

Member

deanberris commented Sep 15, 2013

Yes -- it would be good if I can review the PR so you get feedback sooner on actual code. I'd be happy to see what you've done in a PR, then when we're happy with the implementation/API we can merge accordingly. Shipping early and often is a good thing. :)

@prantlf

This comment has been minimized.

Show comment
Hide comment
@prantlf

prantlf Sep 15, 2013

Okay, I'm going to do it. (Or actually, figuring out how to do it :-)

On 15 September 2013 16:50, Dean Michael Berris notifications@github.comwrote:

Yes -- it would be good if I can review the PR so you get feedback sooner
on actual code. I'd be happy to see what you've done in a PR, then when
we're happy with the implementation/API we can merge accordingly. Shipping
early and often is a good thing. :)


Reply to this email directly or view it on GitHubhttps://github.com/cpp-netlib/cpp-netlib/issues/287#issuecomment-24472815
.

prantlf commented Sep 15, 2013

Okay, I'm going to do it. (Or actually, figuring out how to do it :-)

On 15 September 2013 16:50, Dean Michael Berris notifications@github.comwrote:

Yes -- it would be good if I can review the PR so you get feedback sooner
on actual code. I'd be happy to see what you've done in a PR, then when
we're happy with the implementation/API we can merge accordingly. Shipping
early and often is a good thing. :)


Reply to this email directly or view it on GitHubhttps://github.com/cpp-netlib/cpp-netlib/issues/287#issuecomment-24472815
.

@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Nov 24, 2013

Member

@prantlf Is this ready to make it into 0.11.0? I'm packaging a beta release for testing soon, and I would love to ship this in 0.11.0 by the end of the week.

Member

deanberris commented Nov 24, 2013

@prantlf Is this ready to make it into 0.11.0? I'm packaging a beta release for testing soon, and I would love to ship this in 0.11.0 by the end of the week.

@ghost ghost assigned deanberris Nov 24, 2013

@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Nov 24, 2013

Member

@prantlf So the tests fail to build on OS X with Clang 5.0 (Xcode), Boost 1.54.0, and libc++ -- I'm going to make this a "known issue" until we can find a way to fix this. FWIW I think this is Boost's fault in C++11 mode. I've made some changes to at least make the test build in my current environment, and reference this issue in that commit.

Member

deanberris commented Nov 24, 2013

@prantlf So the tests fail to build on OS X with Clang 5.0 (Xcode), Boost 1.54.0, and libc++ -- I'm going to make this a "known issue" until we can find a way to fix this. FWIW I think this is Boost's fault in C++11 mode. I've made some changes to at least make the test build in my current environment, and reference this issue in that commit.

deanberris added a commit that referenced this issue Nov 27, 2013

leecoder pushed a commit to leecoder/cpp-netlib that referenced this issue Apr 14, 2015

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