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

Writer: Fix bug in last commit; fix old streaming bug on Mac OS X #19

Merged
merged 1 commit into from
Dec 27, 2021

Conversation

evanj
Copy link

@evanj evanj commented Dec 20, 2021

Commit 7f71d02 fixed unsafe slice warnings, but incorrectly
removed a line of code from Writer.nextInputBuffer. This causes the
writer to produce incorrect results. Add a test that reproduces this
bug.

On Mac OS X, this test also often fails even with this bug fix, or
with the version before the bug. It turns out that
LZ4_compress_fast_continue will combine contiguous buffers for small
writes, in an attempt to improve compression. To disable this, we
must separate the buffers. See:

lz4/lz4#473 (comment)

Commit 7f71d02 fixed unsafe slice warnings, but incorrectly
removed a line of code from Writer.nextInputBuffer. This causes the
writer to produce incorrect results. Add a test that reproduces this
bug.

On Mac OS X, this test also often fails even with this bug fix, or
with the version before the bug. It turns out that
LZ4_compress_fast_continue will combine contiguous buffers for small
writes, in an attempt to improve compression. To disable this, we
must separate the buffers. See:

lz4/lz4#473 (comment)
@evanj evanj force-pushed the evan.jones/fix-streaming-bug branch from ab5185b to 05319fc Compare December 20, 2021 18:22
Copy link

@anatolebeuzon anatolebeuzon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix

@evanj evanj merged commit ca07168 into master Dec 27, 2021
@evanj evanj deleted the evan.jones/fix-streaming-bug branch December 27, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants