Rewrite outdated backend notes in lib.rs #481
Conversation
There was a problem hiding this comment.
Thanks a lot for making lib.rs more approachable!
There is only a word that I think should be taken care of, and a question on how some additional context could help dealing with common issues, something the original text might have done but more implicitly.
Thanks again.
src/lib.rs
Outdated
| //! This backend is the fastest, at the cost of some `unsafe` Rust code. | ||
| //! | ||
| //! Several backends implemented in C are also available. | ||
| //! These are useful in the rare cases where you were already using a C implementation |
There was a problem hiding this comment.
I wouldn't assume these cases are rare and turn this into a listing of facts about why the C implementation may be useful. Remove rare would already do it for me.
Further, I'd love it if there was a way to mention typical problem with using any backend. What comes to mind is duplicate symbols, and issue even Rust backends have today, maybe along with a hint at workarounds. The trigger for this paragraph was me thinking zlib-ng-compat was probably good for that, but I wasn't sure.
There was a problem hiding this comment.
Based on the experiments I've run it seemed like zlib-rs did not result in duplicate symbol issues. So you should be able to use the zlib-rs backend without worrying about what other C zlib flavors may be present, and the whole "use C to avoid duplicate symbols" angle should no longer be a thing.
I am not 100% certain about that though, it's been a while. I'll double-check.
Bit-identical output and symbol conflicts are the only reasons to use C backends, right?
There was a problem hiding this comment.
Hmm, I cannot actually reproduce the linker errors due to conflicting symbols with my old setup.
@folkertdev do you know if zlib-rs would cause symbol conflicts if you try to link it into a binary that also links zlib?
There was a problem hiding this comment.
it would cause conflicts right now, but we've added trifectatechfoundation/zlib-rs#322 and will release a new version soon where there should not be any conflicts so long as export-symbols is disabled (which is the default)
|
Thanks a lot! I think it's good enough to merge now, and we can consider improving it further once the new release of |
Do you have anything specific in mind here? E.g. with the symbols it's straightforward I think: flate2 will just not enable the feature flag to export symbols, and if users do want that functionality, they can explicitly depend on libz-rs-sys with the feature enabled (and cargo using the union of features should make that work out). Is there anything else/that I'm missing? |
|
That sounds like a plan - mitigating issues from the start is certainly better than writing about them in |
This documentation historically was out of sync with the README. For example, the version currently live on docs.rs doesn't mention the non-conflicting
zlib-ngfeature at all, even though it's been around for years.Even now the documentation contains outdated statements such as "The compression ratios and performance of each of these feature should be roughly comparable"; these days zlib-ng and zlib-rs are significantly faster than the alternatives.
To avoid similar issues going forward, limit the description to the two most relevant backends, and defer to the README that historically has been kept much more up to date for all the details.