Implement gzdirect #341
Implement gzdirect #341
Conversation
folkertdev
left a comment
folkertdev
left a comment
There was a problem hiding this comment.
Mostly looks good
- I think it'll be more convenient to use
Resultfor private functions that return "success or failure" - there are some blocks that are not covered by tests. This is mostly just fyi, and can be fixed later, but if it's easy to fix now, all the better.
| // No gzip header. If we were decoding gzip before, the remaining bytes | ||
| // are trailing garbage that can be ignored. | ||
| if !state.direct { | ||
| state.stream.avail_in = 0; | ||
| state.eof = true; | ||
| state.have = 0; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
this block is not currently hit by tests
There was a problem hiding this comment.
I was able to cover the other blocks with new tests. This one got messy enough that I put in a FIXME comment for now. The cleanest way to get the GzState into the proper state to trigger this block is probably to finish implementing the read+decompress support, and then create a gzip file with junk content at the end.
|
Thanks for reviewing, I'll post an update later today. Do you have a recommended test coverage measurement tool? I'm looking for something I can easily run locally. |
|
Yes, we use |
|
Is it considered okay to do file I/O from unit tests within |
|
Let's go with that for now, and then see if there is a more elegant way. It might be possible to e.g. fake a file descriptor in some cases, or expose some testing functions with an additional feature flag to |
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 39 files with indirect coverage changes 🚀 New features to boost your workflow:
|
brian-pane
force-pushed
the
gz
branch
2 times, most recently
from
eb8ae1a to
eb2c471
Compare
|
To make my new test cases work under Miri when running locally, I had to use |
|
Oops, I accidentally removed the Miri isolation fix. I'm restoring it now... |
| if state.stream.avail_in != 0 { | ||
| // Copy any remaining input to the start. | ||
| unsafe { | ||
| ptr::copy( |
There was a problem hiding this comment.
I suspect also here
| ptr::copy( | |
| ptr::copy_nonoverlapping( |
There was a problem hiding this comment.
In this case, we're shifting data from the middle of a buffer to the front, so it may overlap.
There was a problem hiding this comment.
Maybe add a note then that they may overlap
libz-rs-sys/src/gz.rs
Outdated
| // Open a non-gzip file for reading. gzdirect should return 1. | ||
| #[test] | ||
| #[cfg_attr(not(target_os = "linux"), ignore = "lseek is not implemented")] | ||
| // Open a gzip file for reading. gzdirect should return 0. |
There was a problem hiding this comment.
something weird happed with the comments here.
3 participants
No description provided.