search

boostorg / iostreams

Boost.org iostreams module
http://boost.org/libs/iostreams
Boost Software License 1.0
43 stars 116 forks source link

Fix processing of multi-stream files #87

Closed gfrontera closed 5 years ago

gfrontera commented 5 years ago

Fixes issue #86

gfrontera commented 5 years ago

Perhaps there is a better fix but this patch fixes the problem for me.

jeking3 commented 5 years ago

Two things:

  1. Is there a test that reasonably exercised this? I'm thinking no, since the original code was not failing tests, so we could use a test.

  2. Rebase on develop and force push so it builds clean with CI fixes. Thanks.

codecov[bot] commented 5 years ago

Codecov Report

Merging #87 into develop will decrease coverage by 0.09%. The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop      #87     +/-   ##
==========================================
- Coverage    69.04%   68.94%   -0.1%     
==========================================
  Files           80       80             
  Lines         3437     3439      +2     
  Branches      1025     1027      +2     
==========================================
- Hits          2373     2371      -2     
- Misses         451      452      +1     
- Partials       613      616      +3
Impacted Files Coverage Δ
include/boost/iostreams/filter/bzip2.hpp 70.27% <93.33%> (-0.57%) :arrow_down:
src/bzip2.cpp 84.9% <0%> (-5.67%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 50b4f00...328a826. Read the comment docs.

gfrontera commented 5 years ago

Thank you for your feedback, @jeking3. It turns out a test did exist. However, the size of the compressed multi-stream was too small for the bug to manifest. The whole multi-stream could fit in the buffer at the same time, and in that case the existing implementation worked correctly. I have just increased the size of the compressed multi-stream (approximately 5x), so now the test fails without this proposed fix.

gfrontera commented 5 years ago

Please @jeking3, could you re-review this pull request to see if the changes made are sufficient?