v2.0.0 #36
v2.0.0 #36
Conversation
There was a problem hiding this comment.
Some detailed comments below. Most of what I say about the compression side applies equally to the decompression side.
The biggest problem I see is with the many overloads of the compress() and decompress() functions. I am not sure the new interface is better than the old class-based one.
include/gzip/compress.hpp
Outdated
| constexpr std::size_t max_step = static_cast<std::size_t>(std::numeric_limits<unsigned int>::max()); | ||
| unsigned int step_size = size > max_step ? max_step : static_cast<unsigned int>(size); | ||
| size -= step_size; | ||
| unsigned int buffer_size = buffering_size > step_size ? step_size : static_cast<unsigned int>(buffering_size); |
There was a problem hiding this comment.
Are there tests that all these calculations and casting creates the right results for 32bit and 64bit systems?
There was a problem hiding this comment.
No I do not have tests for this currently, I should probably add them.
| #pragma GCC diagnostic pop | ||
|
|
||
| std::string buffer; |
There was a problem hiding this comment.
Do we need the extra buffer here? Can't we put the result in output directly?
There was a problem hiding this comment.
I used a buffer here rather than directly writing to output because we need to resize at the end and if multiple loops are required we will be moving a much larger string rather then just reusing the existing buffer and appending it. In the old code this was done by using output.resize(...) followed by another output.resize(...) to shrink the size if necessary.
The problem with my solution is that it might allocate more memory than required though if a buffer size too large is selected. I will continue to think about this.
There was a problem hiding this comment.
Allocations/deallocations are expensive, copying the buffer probably less so. Using an extra buffer will give you more allocations. Not sure how the different sizes of allocations will impact the performance here. In the end you'd have to benchmark to decide this.
| std::size_t size, | ||
| std::string& output, | ||
| int level = Z_DEFAULT_COMPRESSION, | ||
| std::size_t buffering_size = 0) |
There was a problem hiding this comment.
All the different overloads of the compress() function are somewhat confusing and I wonder if they are all correct and unique. The compiler will happily convert between int and size_t and the last parameters have a default value, so I fear in some situation the wrong overload could be chosen or the compiler would detect an ambiguity making the library hard to use. The same applies to the decompress() function below. The original class-based approach had the advantage at least that the configuration parameters were set at construction of the Compressor class.
include/gzip/compress.hpp
Outdated
| } while (deflate_s.avail_out == 0); | ||
| } while (size > 0); | ||
| deflateEnd(&deflate_s); |
There was a problem hiding this comment.
- Return code of
deflateEndis not checked. - If this function throws an exception,
deflateEndis not called and there is a memory leak.
There was a problem hiding this comment.
The only places I could see an exception being thrown would be from std::string::resize or std::string::append. Do you think its worth wrapping it all in a large try catch?
There was a problem hiding this comment.
You could argue that you will only get exceptions if you run out of memory, in which case you are screwed anyways. It would be cleaner to do it correctly, but at least it should be documented, so the next person to change the code knows that this isn't an oversight but a design decision.
Remove the class based approach, provide better ways to use existing string objects. Fixed a variety of other small issues.