Expose `Utf8Lossy` as `Utf8Chunks` #99544

dylni · 2022-07-21T03:30:29Z

This PR changes the feature for Utf8Lossy from str_internals to utf8_lossy and improves the API. This is done to eventually expose the API as stable.

Proposal: rust-lang/libs-team#54
Tracking Issue: #99543

rustbot · 2022-07-21T03:30:32Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-07-21T03:30:33Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

dylni · 2022-07-21T03:31:13Z

@rustbot label +T-libs-api -T-libs

dylni · 2022-08-13T15:57:02Z

r? rust-lang/libs-api
@rustbot

dylni · 2022-08-13T16:04:15Z

Based on rust-lang/highfive#419 and since the previous command isn't working, reassigning randomly:

r? @Mark-Simulacrum

Mark-Simulacrum · 2022-08-14T14:24:52Z

library/core/src/str/lossy.rs

-
-        f.write_char('"')
-    }
-}


It looks like the new Debug impl is just going to format to the byte slice, rather than the (nicer) view we previously had. Maybe we can keep that nicer Debug impl?

The reason I avoided this was that I wasn't sure if the formatting should be Utf8Chunks("...") or "...". However, formatting as "..." could be helpful to avoid the primary use case of bstr::BStr.

For a Debug impl we typically wrap in the struct name, and I think that makes sense in this case too. We could consider a Display impl that formats as "...", though.

The difficulty with using a Display impl instead of Debug for this is that I don't think there's anywhere else in libstd where Display uses byte escapes (except obvious cases such as EscapeDefault), so it might be inconsistent. I would prefer to add something similar to Path::display that would return a new struct.

Based on your later comment, do you want me to squash commits and keep the PR as-is or add the private method described above and use debug_struct to include the struct name?

Ah, I didn't quite realize this wasn't addressed in my quick re-review skim. Let's add the private method and use debug_struct; I think that'll be slightly better. That can be squashed with the other commits though.

Mark-Simulacrum · 2022-08-14T14:27:06Z

library/core/src/str/lossy.rs

    }

-    pub fn chunks(&self) -> Utf8LossyChunksIter<'_> {
-        Utf8LossyChunksIter { source: &self.bytes }
+    /// Returns the invalid character that follows the valid substring and


nit: I guess this isn't really strictly an invalid character, but rather arbitrary bytes? (Of max length 4? Not sure on that, but seems like it might be worth noting).

I clarified this to be "invalid sequence". The result would be a sequence that should have been a character, so calling it a character could be technically wrong.

I added a note about the maximum length as well, but I believe it should be 3 instead of 4. At 4 characters, the sequence should always be parsed as a character.

Mark-Simulacrum · 2022-08-14T14:28:06Z

@rustbot author

Overall looks great, just a couple smaller nits and I think we can go ahead and merge.

dylni · 2022-08-16T03:15:39Z

Thanks for the quick review! All comments should be addressed.

@rustbot ready

Mark-Simulacrum · 2022-08-20T15:28:37Z

r=me with commits squashed, thanks!

dylni · 2022-08-20T16:39:34Z

Sorry for the noise. The new method is not private, but it's as close as I can get, which caused way more issues than I expected.

Diff: https://github.com/rust-lang/rust/compare/fc6e13dd5fd9b46e3b10aca21d0567917236f4e6..f402f48d36af05d9782c8e0c433e1bac05335f9e

@rustbot ready

Mark-Simulacrum · 2022-08-20T16:46:17Z

Seems OK -- I think we should consider a Display impl on Utf8Chunks or some other way to expose this beyond std properly, so maybe if you're interested that's a good idea for an ACP to follow up on this.

@bors r+ rollup

bors · 2022-08-20T16:46:18Z

📌 Commit f402f48d36af05d9782c8e0c433e1bac05335f9e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

dylni · 2022-08-20T16:50:46Z

Sorry about the formatting, it should work now. I didn't consider the rustfmt.toml file changing things when I tested locally.

Mark-Simulacrum · 2022-08-20T16:56:39Z

@bors r+ rollup

bors · 2022-08-20T16:56:41Z

📌 Commit e8ee0b7 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#99415 (Initial implementation of REUSE) - rust-lang#99544 (Expose `Utf8Lossy` as `Utf8Chunks`) - rust-lang#100585 (Fix trailing space showing up in example) - rust-lang#100596 (Remove unnecessary stderr files) - rust-lang#100642 (Update fortanix-sgx-abi and export some useful SGX usercall traits) - rust-lang#100691 (Make `same_type_modulo_infer` a proper `TypeRelation`) - rust-lang#100693 (Add LLVM15-specific codegen test for `try`/`?`s that now optimize away) - rust-lang#100710 (Windows: Load synch functions together) - rust-lang#100807 (Add TaKO8Ki to translation-related mention groups) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned kennytm Jul 21, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2022

rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 21, 2022

dylni mentioned this pull request Jul 21, 2022

Tracking Issue for utf8_chunks #99543

Closed

4 tasks

This comment has been minimized.

Sign in to view

rust-highfive assigned Mark-Simulacrum and unassigned kennytm Aug 13, 2022

Mark-Simulacrum reviewed Aug 14, 2022

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2022

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2022

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 20, 2022

This comment has been minimized.

Sign in to view

Expose Utf8Lossy as Utf8Chunks

e8ee0b7

matthiaskrgr mentioned this pull request Aug 20, 2022

Rollup of 9 pull requests #100809

Merged

bors merged commit d499065 into rust-lang:master Aug 20, 2022

rustbot added this to the 1.65.0 milestone Aug 20, 2022

dylni mentioned this pull request Aug 20, 2022

Expose Utf8LossyChunksIter rust-lang/libs-team#54

Closed

dylni mentioned this pull request Mar 12, 2023

Move Utf8Chunks::new to be inherent on [u8] rust-lang/libs-team#190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `Utf8Lossy` as `Utf8Chunks` #99544

Expose `Utf8Lossy` as `Utf8Chunks` #99544

dylni commented Jul 21, 2022 •

edited

Loading

rustbot commented Jul 21, 2022

rust-highfive commented Jul 21, 2022

dylni commented Jul 21, 2022

This comment has been minimized.

dylni commented Aug 13, 2022

dylni commented Aug 13, 2022

Mark-Simulacrum Aug 14, 2022

dylni Aug 16, 2022

Mark-Simulacrum Aug 20, 2022

dylni Aug 20, 2022

Mark-Simulacrum Aug 20, 2022

Mark-Simulacrum Aug 14, 2022

dylni Aug 16, 2022

Mark-Simulacrum commented Aug 14, 2022

dylni commented Aug 16, 2022

Mark-Simulacrum commented Aug 20, 2022

dylni commented Aug 20, 2022

Mark-Simulacrum commented Aug 20, 2022

bors commented Aug 20, 2022

This comment has been minimized.

dylni commented Aug 20, 2022

Mark-Simulacrum commented Aug 20, 2022

bors commented Aug 20, 2022

Expose Utf8Lossy as Utf8Chunks #99544

Expose Utf8Lossy as Utf8Chunks #99544

Conversation

dylni commented Jul 21, 2022 • edited Loading

rustbot commented Jul 21, 2022

rust-highfive commented Jul 21, 2022

dylni commented Jul 21, 2022

This comment has been minimized.

dylni commented Aug 13, 2022

dylni commented Aug 13, 2022

Mark-Simulacrum Aug 14, 2022

Choose a reason for hiding this comment

dylni Aug 16, 2022

Choose a reason for hiding this comment

Mark-Simulacrum Aug 20, 2022

Choose a reason for hiding this comment

dylni Aug 20, 2022

Choose a reason for hiding this comment

Mark-Simulacrum Aug 20, 2022

Choose a reason for hiding this comment

Mark-Simulacrum Aug 14, 2022

Choose a reason for hiding this comment

dylni Aug 16, 2022

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 14, 2022

dylni commented Aug 16, 2022

Mark-Simulacrum commented Aug 20, 2022

dylni commented Aug 20, 2022

Mark-Simulacrum commented Aug 20, 2022

bors commented Aug 20, 2022

This comment has been minimized.

dylni commented Aug 20, 2022

Mark-Simulacrum commented Aug 20, 2022

bors commented Aug 20, 2022

Expose `Utf8Lossy` as `Utf8Chunks` #99544

Expose `Utf8Lossy` as `Utf8Chunks` #99544

dylni commented Jul 21, 2022 •

edited

Loading