8000 Update LZ4 to 1.10.0 by Sewer56 · Pull Request #25 · picoHz/lzzzz · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update LZ4 to 1.10.0 #25

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 6, 2024
Merged

Update LZ4 to 1.10.0 #25

merged 5 commits into from
Dec 6, 2024

Conversation

Sewer56
Copy link
Contributor
@Sewer56 Sewer56 commented Nov 10, 2024 •
edited
Loading

fixes #23

Apologies, getting around to this took a bit longer than expected.
Current v1.10.0 API availability:

-✅ LZ4_loadDictSlow()
-✅ LZ4_attach_dictionary()
-✅ LZ4_attach_HC_dictionary()
-❌ LZ4F_compressBegin_usingDict() [not strictly needed]
-✅ LZ4F_decompress_usingDict() [already existed]
-✅ LZ4F_createCDict() [already existed]
-❌ LZ4F_compressFrame_usingCDict()
-✅ LZ4F_compressBegin_usingCDict() [already existed]

@Sewer56
Copy link
Contributor Author
Sewer56 commented Nov 10, 2024

I caught a bug while doing LZ4_attach_HC_dictionary.
The documentation for LZ4 states

// In order for LZ4_loadDictHC() to create the correct data structure,
// it is essential to set the compression level before loading the dictionary.

The current implementation of with_dict for LZ4HC actually doesn't work due to this.
Will fix in this branch.

@Sewer56
Copy link
Contributor Author
Sewer56 commented Nov 10, 2024

Regardless of the above, it seems with_dict still doesn't produce the same file as with attaching a dict.
I'm starting to believe it's an LZ4 upstream bug at this point.

https://github.com/lz4/lz4/blob/7887022e5d1de41f1cf53bb12a646d13d6e278d4/tests/fuzzer.c#L936-L938

The wrapper is doing the same as LZ4's fuzzer tests, for loading with dict.
The attach case works, the regular does not, oddly.

@Sewer56 Sewer56 mentioned this pull request Nov 10, 2024
Open
@Sewer56
Copy link
Contributor Author
Sewer56 commented Nov 10, 2024 •
edited
Loading

Submitted the oddity I found as:

To upstream.

@Sewer56 Sewer56 marked this pull request as ready for review November 10, 2024 05:35
@Sewer56 Sewer56 changed the title [WIP] Update LZ4 to 1.10.0 Update LZ4 to 1.10.0 Nov 10, 2024
@picoHz
Copy link
Owner
picoHz commented Nov 11, 2024

@Sewer56 Thanks for your PR!
It looks like there are some clippy issues. Could you take a look?

@Sewer56
Copy link
Contributor Author
Sewer56 commented Nov 11, 2024

Yup, I'll fixup later today.
I never noticed because I usually run it on save in my repos, but that wasn't enabled here.

@Sewer56
Copy link
Contributor Author
Sewer56 commented Dec 5, 2024 •
edited
Loading

I've been really bad at getting this done in a timely manner 😅
Sorry about that, hard to juggle so much code work; between work and own projects, endlessly.

In any case, I fixed up the clippy lints.
They weren't related to this PR, but it should hopefully pass CI.

At least locally I get no warnings.

@picoHz
Copy link
Owner
picoHz commented Dec 6, 2024

@Sewer56 Thank you for all your hard work. Sorry, I thought the warnings were related to your PR.

@picoHz picoHz merged commit f7ac59f into picoHz:master Dec 6, 2024
12 checks passed
@Sewer56
Copy link
Contributor Author
Sewer56 commented Dec 6, 2024

👏 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maintenance] Update LZ4 to 1.10.0
2 participants
@Sewer56 @picoHz
0