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

Rewrite UTF8Validator #23

Merged
merged 10 commits into from
Jan 25, 2016
Merged

Rewrite UTF8Validator #23

merged 10 commits into from
Jan 25, 2016

Conversation

SimonSapin
Copy link
Member

This introduces two (recursive) dependencies that should probably be reviewed as well:

r? @nox

Review on Reviewable

@asajeffrey
Copy link
Member

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/stream.rs, line 46 [r1] (raw file):
Bikeshedding: should Utf8LossyDecoder and Decoder follow the same naming scheme? (That is, either Utf8LossyDecoder becomes LossyDecoder or Decoder becomes Utf8Decoder.)


src/stream.rs, line 90 [r1] (raw file):
Add an assert! for this?


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

I did a brief look over the new dependencies, and they look fine. They could both do with a LICENSE file as well as a Cargo.toml file (not all lawyers know their way around Cargo).

* Rename it to `Utf8LossyDecoder`
* Don’t allocate any memory.
* Handles errors per [Unicode Standard §5.22 "Best Practice for U+FFFD
  Substitution"](http://www.unicode.org/versions/Unicode8.0.0/ch05.pdf#G40630)
  This matches `String::from_utf8_lossy`.

This introduces two (recursive) dependencies that should probably
be reviewed as well:

* [rust-utf8](https://github.com/SimonSapin/rust-utf8)
* [string-wrapper](
  https://github.com/SimonSapin/rust-std-candidates/tree/master/string-wrapper)
SimonSapin added a commit to servo/html5ever that referenced this pull request Jan 25, 2016
@SimonSapin
Copy link
Member Author

I’ve pushed some more changes and used them in html5ever: servo/html5ever#188


Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.


src/stream.rs, line 46 [r1] (raw file):
I’ve renamed Decoder to LossyDecoder. The difference between the two is UTF-8 v.s. any given encoding, not lossy v.s. not.


src/stream.rs, line 90 [r1] (raw file):
subtendril is safe: it checks that the given offset and length are within bounds and panics if they’re not. An assert would only panic earlier in the same cases.


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/fmt.rs, line 182 [r3] (raw file):
The comments for this trait talk about Format, which used to be an associated type, but has now vanished.


Comments from the review on Reviewable.io

SimonSapin added a commit to servo/html5ever that referenced this pull request Jan 25, 2016
@asajeffrey
Copy link
Member

@bors-servo r+

@SimonSapin
Copy link
Member Author

@bors-servo r=asajeffrey

@asajeffrey
Copy link
Member

@bors-servo r+

@SimonSapin
Copy link
Member Author

Travis is not feeling well:

https://travis-ci.org/servo/tendril/jobs/104723799

Installing Rust

0.94s$ curl -sL https://static.rust-lang.org/rustup.sh -o ~/rust-installer/rustup.sh

$ sh ~/rust-installer/rustup.sh --prefix=~/rust --spec=nightly -y --disable-sudo 2> /dev/null

rustup: gpg available. signatures will be verified

rustup: downloading manifest for 'nightly'

rustup: downloading toolchain for 'nightly'

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

I’ve run tests locally on all three Rust release channels.

SimonSapin added a commit that referenced this pull request Jan 25, 2016
@SimonSapin SimonSapin merged commit 9c10d1f into master Jan 25, 2016
@SimonSapin SimonSapin deleted the lossy-utf8 branch January 25, 2016 20:09
SimonSapin added a commit to servo/html5ever that referenced this pull request Jan 25, 2016
bors-servo pushed a commit to servo/html5ever that referenced this pull request Jan 26, 2016
Rewrite the high-level API (driver module) to use TendrilSink

This depends on servo/tendril#23.

This also adds an API to parse from bytes, which is part of #18.

r? @nox

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/188)
<!-- Reviewable:end -->
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