Skip to content

Code review #27

@joto

Description

@joto

I looked at the code and have some comments:

  1. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/compress.hpp#L27 That's confusing: The output parameter is of type InputType? And I would have expected the output parameter to come after the input parameters. But that's historically inconsistent in C APIs anyway...
  2. Not sure we need the Compressor and Decompressor classes. The only thing that ever happens to them is that they are instantiated and then the (de)compress function is called on them. That would work as a simple function? I can see why this might be useful when we (de)compress in blocks, but the library can't do that due to the inflateInit2 and deflateInit2 functions not being in the constructor. Okay, we don't have to set those compression parameters every time, but how often do we re-use an instantiated Compressor or Decompressor object to make this useful?
  3. Useless SECTION: https://github.com/mapbox/gzip-hpp/blob/master/test/test_io.cpp#L12
  4. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/utils.hpp#L8 can be noexcept and the function is hard to follow due the many parentheses.
  5. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/compress.hpp#L32-L38 Consider using assert() here.
  6. There should be helper functions like (de)compress() that take an existing std::string and fill it.
  7. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/decompress.hpp#L64 If the second condition is true, the first is also true, so this can be simplified.
  8. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/decompress.hpp#L57 I don't think this is correct. If std::size is 32bit size * 2 will be 32bit and can overflow before it is assigned to the 64bit size_64. const auto size_64 = static_cast<uint64_t>(size) * 2; should be correct.
  9. There are several places where inflateEnd is called (like here https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/decompress.hpp#L60). Maybe this can be wrapped in an RAII wrapper somehow?
  10. If we have gzipped-compressed data, the header can tell us how large the uncompressed data was. Can we use this to optimize this case and have the output buffer in the correct size from the beginning?
  11. https://github.com/mapbox/gzip-hpp/blob/master/include/gzip/decompress.hpp#L55-L63 this can be moved above the inflateInit2 call saving an inflateEnd. Also consider making this an assert.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions