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

Redefine Serializer::collect_str #267

Merged
merged 4 commits into from
Mar 6, 2017
Merged

Redefine Serializer::collect_str #267

merged 4 commits into from
Mar 6, 2017

Conversation

nox
Copy link
Contributor

@nox nox commented Feb 27, 2017

No description provided.

@nox
Copy link
Contributor Author

nox commented Feb 27, 2017

Related to serde-rs/serde#789, do not merge yet.

json/src/ser.rs Outdated
{
fn write_str(&mut self, s: &str) -> fmt::Result {
assert!(self.error.is_none());
match self.formatter.write_string_fragment(&mut self.writer, s.as_bytes()) {
Copy link
Member

Choose a reason for hiding this comment

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

The write_string_fragment method dumps bytes directly into the output. Instead you need to factor out the middle part of format_escaped_str and use that here to properly escape any special characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made write_string_fragment take a &str and contain the whole loop from the function format_escaped_str.

@nox nox force-pushed the display branch 2 times, most recently from 540a382 to 46ea736 Compare February 27, 2017 09:57
@nox
Copy link
Contributor Author

nox commented Feb 27, 2017

I also added a commit to optimise serialising a single char.

@dtolnay
Copy link
Member

dtolnay commented Feb 28, 2017

This is a breaking change for rq which overrides write_string_fragment: rq/value/json.rs

Please do not make any changes to Formatter in this PR. The loop for calling write_string_fragment / write_char_escape should live outside of Formatter.

I filed #268 to follow up on the argument type in the next major release.

json/src/ser.rs Outdated
let mut buf = [0; 4];
write!(&mut buf[..], "{}", value).unwrap();
let str = unsafe { str::from_utf8_unchecked(&buf[0..value.len_utf8()]) };
format_escaped_str(wr, formatter, str)
Copy link
Member

Choose a reason for hiding this comment

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

Please do not call any variables str. GitHub shows it as a keyword and makes it look like a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to slice.

json/src/ser.rs Outdated
use std::io::Write;
let mut buf = [0; 4];
write!(&mut buf[..], "{}", value).unwrap();
let str = unsafe { str::from_utf8_unchecked(&buf[0..value.len_utf8()]) };
Copy link
Member

Choose a reason for hiding this comment

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

Add a brief justification of why this is safe along with a link to https://github.com/serde-rs/json/issues/270.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added that.

This allows us to format_args!() and friends while avoiding String
allocations when serialising to JSON.
@nox
Copy link
Contributor Author

nox commented Feb 28, 2017

Note that if the Formatter methods produced io::Error errors, or at least if the portion that handles strings did, we wouldn't need to implement fmt::Write and store a serde_json::Error out of band, instead we could use write! directly and use the returned io::Error.

@dtolnay dtolnay merged commit 97c184f into serde-rs:master Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants