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

Remove unnecessary unsafe in Text::poll #559

Closed
wants to merge 1 commit into from
Closed

Remove unnecessary unsafe in Text::poll #559

wants to merge 1 commit into from

Conversation

64
Copy link

@64 64 commented Jul 7, 2019

Cow supports .into_owned() which does the same thing. I'm unclear whether the code was like that deliberately as a performance choice, or something else I'm missing?

@64
Copy link
Author

64 commented Jul 7, 2019

CI failures don't seem related?

@messense
Copy link
Contributor

messense commented Jul 8, 2019

#256 (comment)

@64
Copy link
Author

64 commented Jul 8, 2019

Urgh, that makes sense. I totally forgot that it moves the Vec. I’ll play around with some benchmarks later, perhaps.

@64
Copy link
Author

64 commented Jul 8, 2019

Side note, why is encoding_rs being used as opposed to from_utf8_lossy?

@messense
Copy link
Contributor

messense commented Jul 8, 2019

Side note, why is encoding_rs being used as opposed to from_utf8_lossy?

HTTP response body isn't always UTF-8 encoded.

@64
Copy link
Author

64 commented Jul 8, 2019

Doesn’t from_utf8_lossy do the same thing of replacing invalid UTF-8 sequences with the replacement character?

@messense
Copy link
Contributor

messense commented Jul 8, 2019

Doesn’t from_utf8_lossy do the same thing of replacing invalid UTF-8 sequences with the replacement character?

That's not what encoding_rs does, it decodes other encoding, say GBK, to UTF-8 because Rust String is UTF-8 encoded.

https://en.wikipedia.org/wiki/Character_encoding

https://en.wikipedia.org/wiki/Character_encodings_in_HTML

@64
Copy link
Author

64 commented Jul 8, 2019

Ok, my mistake. Closing for now

@64 64 closed this Jul 8, 2019
@seanmonstar
Copy link
Owner

While it didn't work out here, I appreciate the effort!

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.

3 participants