Conversation
I wrote independent tests for the original base64 module; this upstreams them and fixes a few bugs that my original tests exposed in the new base64 library. Also fix a few bugs that I discovered when running the tests in valgrind.
|
Ping |
|
@trainman419, just to give you an expectation, @dirk-thomas is still on vacation, perhaps @mikepurvis has some time but I wouldn't expect it to be merged until later next week. |
|
Ping. @dirk-thomas or @mikepurvis do you have a moment to take a look at this, #1243 and #1244? |
mikepurvis
left a comment
There was a problem hiding this comment.
LGTM, just nits and questions.
| "FiY2RlZmdoaWpr\nbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6PkJ" | ||
| "GSk5SVlpeYmZqbnJ2en6Ch\noqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/wM" | ||
| "HCw8TFxsfIycrLzM3Oz9DR0tPU1dbX\n2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v8P" | ||
| "Hy8/T19vf4+fr7/P3+/w==\n"))); |
There was a problem hiding this comment.
Is this a golden test copied from somewhere, eg an online converter? If so, might be worth including an attribution in case someones want to change or reproduce it later.
There was a problem hiding this comment.
I generated this locally from the binary version of the sequence above, using the base64 program.
| { | ||
| state_in->step = step_a; | ||
| state_in->plainchar = *plainchar; | ||
| state_in->plainchar = 0; // no state to save; use default value |
There was a problem hiding this comment.
I don't know enough about base64 to understand this change, but I assume it's motivated by something which one of the new tests uncovered?
There was a problem hiding this comment.
Yes; this was uncovered by running these tests in valgrind and prevents an out-of-bounds array access through plainchar.
| char* plainchar = plaintext_out; | ||
| char fragment; | ||
|
|
||
| if(length_in == 0) return 0; |
There was a problem hiding this comment.
if (length_in == 0)
{
return 0;
}
|
|
||
| catkin_add_gtest(test_base64 test_base64.cpp) | ||
| target_link_libraries(test_base64 xmlrpcpp) | ||
| set_target_properties(test_base64 PROPERTIES COMPILE_FLAGS -std=c++11) |
There was a problem hiding this comment.
Should this be per-target vs. setting it for the whole package with set_directory_properties?
There was a problem hiding this comment.
This is the only test that needs C++11, so I figured the cleanest thing to do was enable it on this target.
|
Looks good to me. Thanks for contributing new tests! |
I wrote independent tests for the original base64 module; this upstreams them and fixes a few bugs that my original tests exposed in the new base64 library.
Also fix a few bugs that I discovered when running the tests in valgrind, and remove an unused variable in the base64 decoder.