Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix processing of multi-stream files #87

Merged
merged 3 commits into from May 24, 2019

Conversation

@gfrontera
Copy link
Contributor

commented Apr 23, 2019

Fixes issue #86

@gfrontera

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

@jeking3

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

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.

@jeking3
Copy link
Collaborator

left a comment

We could use a test with input that leverages multi-stream files.

@codecov

This comment has been minimized.

Copy link

commented May 9, 2019

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%) ⬇️
src/bzip2.cpp 84.9% <0%> (-5.67%) ⬇️

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

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

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

@jeking3
Copy link
Collaborator

left a comment

Looks like it now:

meet the postcondition requiring that either i1 == i2 or o1 == o2

described in the related issue.

@jeking3 jeking3 merged commit d6301d8 into boostorg:develop May 24, 2019

4 checks passed

codecov/patch 93.33% of diff hit (target 69.04%)
Details
codecov/project Absolute coverage decreased by -0.09% but relative coverage increased by +24.29% compared to 50b4f00
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gfrontera gfrontera deleted the gfrontera:patch-1 branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.