feat: Upgrade [Extended]FileOptions::add_extra_data() data from Box<[u8]> to AsRef<[u8]> #435
Conversation
Summary of ChangesHello @nabijaczleweli, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors [Extended]FileOptions::add_extra_data() to be more flexible and performant by accepting AsRef<[u8]> instead of Box<[u8]>. This is a great improvement as it allows callers to avoid unnecessary heap allocations, as demonstrated in the updated call sites. The changes are well-implemented and improve the library's API. I've found one minor issue that prevents compilation, which I've commented on. Once that's fixed, this should be good to merge.
…AsRef<[u8]> Existing callers are unaffected but you can just give it slices now
nabijaczleweli
force-pushed
the
master
branch
from
5c66812 to
1831396
Compare
Pr0methean
left a comment
Pr0methean
left a comment
There was a problem hiding this comment.
Looks good in principle, but I have one question.
src/write.rs
Outdated
| ) -> ZipResult<()> { | ||
| self.add_extra_data_impl(header_id, data.as_ref(), central_only) | ||
| } | ||
| fn add_extra_data_impl( |
There was a problem hiding this comment.
Why split this method? It seems to me it shouldn't occupy more than about 1KB compiled, so the cost of inlining it into 2 or 3 generic specializations should be very reasonable.
There was a problem hiding this comment.
No reason not to, I suppose? I can unsplit this if you want.
| body.push(mode as u8); // strength | ||
| body.write_u16_le(underlying.serialize_to_u16())?; // real compression method | ||
| let mut body = [0; 7]; | ||
| [body[0], body[1]] = (vendor as u16).to_le_bytes(); // vendor version (1 or 2) |
There was a problem hiding this comment.
It looks from the build errors like this may need to be body[0..=1] instead.
There was a problem hiding this comment.
That was complaining about wanting = *b"AE" actually ([body[0], body[1]] is already used elsewhere in the file); I think I didn't test with all the right feature combos locally, should all be fixed.
Pr0methean
changed the title
|
Looks like this will break backward-compatibility, and Cargo rules say that requires a new major release. Try bumping the version to 6.0.0. |
2 participants
This doesn't affect existing callers (
whatever.add_extra_data(a, b, c)whereb: Box<[u8]>continues to work) but it lets new ones not heap-allocate slices which are, in reality, borrowed.This lets the internal
ExtendedFileOptions::add_extra_data_unchecked()interface take a borrowed slice and 2/3 callers are trivially upgraded to borrowing stack slices instead of allocating.