Proposed-BinaryBufferBase64

[MakoEnergy]

These are the ported versions of the BinaryBuffer class and Base64 suite of methods.

[MakoEnergy]

Making a note here and now to make issues for the planned replacement of some code in this PR. Once we have exceptions the std::exception throws in Base64.cpp should be replaced with our own exceptions. Additionally, once we have a SUPPRESS_ALL_WARNINGS the explicit warning suppressions around the external code we use to benchmark Base64 encodes and decodes should be replaced.

[MakoEnergy]

Currently all the UnitTests pass on my local machine, however I'm seeing an anomaly in my timings. Occasionally I'll see the BinaryBuffer tests taking zero time. To clarify, I mean zero in all time fields in the final report, including "ns". This only ever changes on rebuilds, and only sometimes. The one time I tested a rebuild with no changes it didn't cause a change in the timing behavior. Is it possible the optimizer is optimizing out every test?

[codecov[bot]]

Codecov Report

Merging #45 into master will decrease coverage by 1.15%. The diff coverage is 93.38%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   98.49%   97.33%   -1.16%     
==========================================
  Files           9       12       +3     
  Lines         465      601     +136     
==========================================
+ Hits          458      585     +127     
- Misses          7       16       +9

[codecov[bot]]

Codecov Report

Merging #45 into master will increase coverage by 0.33%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   98.49%   98.83%   +0.33%     
==========================================
  Files           9       12       +3     
  Lines         465      599     +134     
==========================================
+ Hits          458      592     +134     
  Misses          7        7

[MakoEnergy]

I fixed all the code related bugs CI has found. I have a couple of functions I want to try and cleanup, and then I have code coverage additions to make. Then the PR should be ready.

[Sqeaky]

This is my sin originally, but in Base64.cpp around line 120 There should be a comment about the assumptions in this code. The function Mezzanine::Base64::IsBase64(const Char8) assumes that the letters are in alphabetic order (as opposed) in the encoding. This holds true for current popular encodings like ASCII and UTF-8. I don't foresee an encoding that would change this so I advise no functional code change, but a comment would probably be good.

We probably only need to be worried if someone make a new popular text encoding base on huffman coding or something.

[Sqeaky]

I think the docs of the move constructor on the binary buffer should include more details. I haven't read that far, but presumably the pointers are swapped and the sizes are swapped, and knowing that here would be nice.

If the move does work like that, then the warning about deep copies ought to be removed from the class documentation.

[Sqeaky]

Some of the lines in the Binary buffer header are quite long. Line 167 is:

/// @param NewSize If you don't want to just clear the buffer, but rather want to set size to a value and set a new size, you can do that with this.

[Sqeaky]

The docs on BinaryBuffer::DeleteBuffer(const SizeType) to state that it will always set the size to 0 when it actually defaults to 0. This should be amended.

[Sqeaky]

What happens with nulls in inside the buffer when we call BinaryBuffer::ToString? Does std::string cope with that?

[Sqeaky]

BinaryBuffer::BinaryBuffer(BinaryBuffer&& Other) Should probably swap pointers.

In its current state it takes the pointer and assigns the other to 0 and nullptr. If the current buffer was valid this is now leaked. There are two strategies for handling this. First, delete the local buffer then take the other's pointer/size. Second, swap both sizes and both pointers and defer deallocation to the destruction of either object.

I think the swapping is preferable because expensive operations like deallocations are expected on destruction and moves are generally expected to be cheap.

EDIT - In BinaryBuffer::operator=(BinaryBuffer&&) the deallcation is to delete the current buffer then copy pointer/size, swapping might be preferable here as well.

[MakoEnergy]

1) I'm not sure I follow. The ordering of the checks is there for no particular reason...just the order I read off of the ascii chart. If a certain grouping is more likely I can reorder the checks for the possible performance boost...but that wouldn't need to be in the docs. The check itself assumes no ordering on the part of the data...not sure how it could given it accepts a single char.

2) I do not feel that moving is swapping, and there being distinct swap and move functions in the standard agrees with me I think. Moving isn't necessarily meant to be fast, just faster than a copy. Or, at absolute worst, the same as a copy. In this case the move assignment is faster than copy assignment. Either would would cause a delete, but the move doesn't require a memcpy call. Additionally, the behavior of some standard containers (such as string and vector) in my experience cause the source container to revert to the same status as calling the default constructor (no data, but ready/usable). Some additional reading to consider: https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty I've touched up the docs to clarify it's behavior.

3) Fixed.

4) Fixed.

5) Yikes, it does not. I have fixed it but I'm also wondering if it's worth it to have that method at all.

6) As we discussed, there is no leak in the constructor. I think the constructor behavior is fine given a swap would be functionally identical to what I am already doing. The move assignment is a different story. For that, I feel I've said as much as I can in a single comment about that in "2)".

[Sqeaky]

On 1 the order of the checks is unimportant. I meant that not all encodings will keep all the letters together. The Ascii chart works for Ascii and UTF8 because those two encodings are specified to be compatible. This might change in some unlikely distant future.

[Sqeaky]

On 5 I think we talked passed our points again. I was curious about null terminators in the middle of the binary buffer. Seems like it would be fine, but I will read up.

[Sqeaky]

On line 125 test/Base64Tests.h "@ref" should probably be "@return" credit to @HLayne35.