Skip to content
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.

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

clean up and readme edit #34

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertnurnberg
Copy link
Contributor

This PR offers a cleaner implementation of the new istreambuf constructor introduced in #32. We also add a new overloaded ifstream constructor, allowing the simpler syntax

bxz::ifstream input(file, bxz::zstd);

as for ofstream.

Also edit the readme.

@robertnurnberg
Copy link
Contributor Author

Btw, with the new none compression method the old auto_detect parameter for the istreambuf constructor could be dropped completely. Going either for the syntax

istreambuf(std::streambuf * _sbuf_p, Compression _type = none, std::size_t _buff_size = default_buff_size)

or

istreambuf(std::streambuf * _sbuf_p, std::size_t _buff_size = default_buff_size, Compression _type = none)

And with the convention that none means we attempt auto detection.

Let me know if you think this is a good idea and which of the two syntax you would prefer. (Only the latter would give backward compatibility.) I could then prepare a PR.

@tmaklin
Copy link
Owner

tmaklin commented Feb 3, 2025

Thanks again, I need to do a major version bump anyway to fix #23 and #27 so the first one looks better to me. Feel free to make a PR if you like.

@robertnurnberg
Copy link
Contributor Author

robertnurnberg commented Feb 3, 2025

Can do the PR as discussed once this is merged. See #35 for an implementation.

@robertnurnberg robertnurnberg marked this pull request as draft February 3, 2025 18:06
@robertnurnberg
Copy link
Contributor Author

Changing this to draft in favour of #35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants