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

Add Utf8Error::error_len, to help incremental and/or lossy decoding. #40212

Merged
merged 3 commits into from
Mar 15, 2017

Conversation

SimonSapin
Copy link
Contributor

Without this, code outside of the standard library needs to reimplement most of the logic from_utf8 to interpret the bytes after valid_up_to().

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@SimonSapin
Copy link
Contributor Author

This is a new standard library public API that I’m hoping to stabilize eventually. But it’s a small extension of an existing feature rather than an entirely new feature, so it seemed appropriate to submit without an RFC.

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 2, 2017
@sfackler
Copy link
Member

sfackler commented Mar 2, 2017

This seems reasonable to me and I agree that an RFC isn't necessary. cc @rust-lang/libs

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 2, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@bors
Copy link
Contributor

bors commented Mar 3, 2017

☔ The latest upstream changes (presumably #40189) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin
Copy link
Contributor Author

Rebased.

@SimonSapin SimonSapin changed the title Add Utf8Error::resume_from, to help incremental and/or lossy decoding. Add Utf8Error::error_len, to help incremental and/or lossy decoding. Mar 6, 2017
@SimonSapin
Copy link
Contributor Author

After trying it I’ve pushed another commit to change the proposed API:

Replace Utf8Error::resume_from with Utf8Error::error_len

Their relationship is:

  • resume_from = error_len.map(|l| l + valid_up_to)
  • error_len is always one of None, Some(1), Some(2), or Some(3).

When I started using resume_from I almost always ended up subtracting valid_up_to to obtain error_len. Therefore the latter is what should be provided in the first place.

re r? @BurntSushi @Kimundi @alexcrichton @aturon @brson @sfackler

@rust-highfive rust-highfive assigned BurntSushi and unassigned sfackler Mar 6, 2017
@alexcrichton
Copy link
Member

Seems fine by me!

@aturon
Copy link
Member

aturon commented Mar 13, 2017

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 13, 2017

📌 Commit 354f092 has been approved by aturon

@SimonSapin
Copy link
Contributor Author

Added a tracking issue number to the PR. @aturon re-r?

@alexcrichton
Copy link
Member

@bors: r=aturon

@bors
Copy link
Contributor

bors commented Mar 13, 2017

📌 Commit 5b3dbd8 has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 14, 2017

🔒 Merge conflict

Without this, code outside of the standard library needs to reimplement
most of the logic `from_utf8` to interpret the bytes after `valid_up_to()`.
Their relationship is:

* `resume_from = error_len.map(|l| l + valid_up_to)`
* error_len is always one of None, Some(1), Some(2), or Some(3).

When I started using resume_from I almost always ended up subtracting
valid_up_to to obtain error_len.
Therefore the latter is what should be provided in the first place.
@SimonSapin
Copy link
Contributor Author

Rebased.

I don’t understand how this happened, but git seemed convinced that my commits wanted to remove the src/doc/reference and src/doc/nomicon submodules that I think didn’t exist at the time and have been added since. This was the only "conflict".

@aturon or @alexcrichton r?

@alexcrichton
Copy link
Member

alexcrichton commented Mar 14, 2017

~submodules~

@bors: r=aturon

@bors
Copy link
Contributor

bors commented Mar 14, 2017

📌 Commit 73370c5 has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 14, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Mar 14, 2017

📌 Commit 73370c5 has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 14, 2017

⌛ Testing commit 73370c5 with merge ae118b1...

@frewsxcv
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Mar 15, 2017

⌛ Testing commit 73370c5 with merge 11a3376...

bors added a commit that referenced this pull request Mar 15, 2017
Add Utf8Error::error_len, to help incremental and/or lossy decoding.

Without this, code outside of the standard library needs to reimplement most of the logic `from_utf8` to interpret the bytes after `valid_up_to()`.
@bors
Copy link
Contributor

bors commented Mar 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 11a3376 to master...

@bors bors merged commit 73370c5 into rust-lang:master Mar 15, 2017
@SimonSapin SimonSapin deleted the utf8error-resume-from branch March 15, 2017 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants