Drop unused Seek bounds from Decoder and Reader#687
Conversation
Relax input requirements by removing the unused `Seek` bound from the `Decoder` and `Reader` APIs, as suggested in a review comment on PR image-rs#684. Additionally: - Cleaned up unit tests and fuzz targets to remove mock `Seek` implementations. - Removed unused `TryFrom` import in `src/encoder.rs` to resolve compiler warnings.
197g
left a comment
197g
left a comment
There was a problem hiding this comment.
Neat, this simplifying a lot of test shims is great news for consumers of the API. We previously had added Seek to existing functions out of consistency concerns. We're going to revise that here: we haven't seen much utilization of the bound during decoding. Reason mostly being that real images seldom include wasteful data by design, skipping over most auxiliary chunks by reading them is cheap enough.
Also since then I've myself become quite a lot more critical of IO trait bounds. They are viral and we can not express them as optional bounds. (The rather unsafe flexible-io experiment not-withstanding). In any case we can re-introduce optional Seek through a caller provided Option<Arc<dyn Fn(&mut R) -> &mut dyn Seek>>; seeking should not be as performance sensitive for inlining as Read so dyn trait seems fine. This is more flexible in any case.
3 participants
Relax input requirements by removing the unused
Seekbound from theDecoderandReaderAPIs, as suggested in a review comment on PR #684.Additionally:
Seekimplementations.TryFromimport insrc/encoder.rsto resolve compiler warning on CI.