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

Implement fmt::Write for CxxString #1202

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

parasyte
Copy link
Contributor

@parasyte parasyte commented Mar 23, 2023

This allows formatted writes (e.g. with the write!() macro) directly on a CxxString. This can eliminate a needless allocation from formatting to a String then converting to a CxxString.

While testing this, I ran into #1201. Nothing else stood out as unusual.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether std::fmt::Write or std::io::Write is more appropriate. In the Rust standard library, String implements fmt::Write but not io::Write, while Vec<u8> implements io::Write and not fmt::Write. CxxString has similarities to both in different ways.

If we add fmt::Write now then adding io::Write later is a breaking change because it would make cxx_string.write_fmt(…) ambiguous in the case that you have both traits in scope.

Thoughts?

@parasyte
Copy link
Contributor Author

parasyte commented Mar 24, 2023

Great observation! I can add an impl for io::Write on CxxVector<u8> to this PR. That should address the semver hazards. Are there any other concerns that come to mind?

Ah, and I did not consider implementing io::Write on CxxString, but I could see an argument for it.

@parasyte
Copy link
Contributor Author

@dtolnay I have addressed the concerns with breaking changes by implementing both fmt::Write and io::Write. I decided not to do anything with CxxVector because I don't need it and it's more work to test.

This allows formatted writes (e.g. with the `write!()` macro) directly on a CxxString. This can eliminate a needless allocation from formatting to a String and converting to a CxxString.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay dtolnay merged commit 9e317fc into dtolnay:master Mar 25, 2023
@parasyte parasyte deleted the feature/fmt-write branch March 25, 2023 20:50
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.

None yet

2 participants