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

new companion method to next_code_point() that accepts Iterator<Item=u8> #95788

Closed
wants to merge 6 commits into from

Conversation

mutantbob
Copy link

Since next_code_point() requires an Iterator<Item=&u8> and I only have an Iterator<Item=u8> I created next_code_point_val().
It is really just the same code, with the original method changed to invoke next_code_point_val(&mut bytes.copied()) . I threw in some unit tests because I want some confidence that I didn't break anything.

@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 @dtolnay (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2022
@bors
Copy link
Contributor

bors commented Apr 8, 2022

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

@bors
Copy link
Contributor

bors commented Apr 9, 2022

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

@mutantbob
Copy link
Author

would resolve #95940

@@ -34,8 +34,21 @@ pub(super) const fn utf8_is_cont_byte(byte: u8) -> bool {
#[unstable(feature = "str_internals", issue = "none")]
#[inline]
pub unsafe fn next_code_point<'a, I: Iterator<Item = &'a u8>>(bytes: &mut I) -> Option<u32> {
// SAFETY: `bytes` must produce a valid UTF-8-like (UTF-8 or WTF-8) string
unsafe { next_code_point_val(&mut bytes.copied()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

considering that next_code_point is unstable, it's probably better to change next_code_point to take Iterator<Item = u8> directly. Any use can get fixed by using copied().

Copy link
Author

Choose a reason for hiding this comment

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

I had considered that, but I wanted to minimize my impact, mostly because the process of running the unit tests takes a really long time. I can create a second branch that alters next_code_point to see if that version is preferred by folks with merge authority.

Copy link
Author

Choose a reason for hiding this comment

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

#96019 is a variant that modifies next_code_point.

Copy link
Member

@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.

Closing in favor of #96019.

Like Mara wrote in the other PR, I don't think either of these changes is going to be acceptable.

@dtolnay dtolnay closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants