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

EncodeWide should implement FusedIterator #96368

Closed
AronParker opened this issue Apr 24, 2022 · 5 comments · Fixed by #96397
Closed

EncodeWide should implement FusedIterator #96368

AronParker opened this issue Apr 24, 2022 · 5 comments · Fixed by #96397
Assignees
Labels
O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@AronParker
Copy link
Contributor

AronParker commented Apr 24, 2022

EncodeWide iterates over Wtf8CodePoints (which point to a slice of bytes in memory). When the code points expire (no more bytes available in the slice), there is no chance of the iterator ever returning more u16 values. Therefore, EncodeWide should implement FusedIterator.

@ChrisDenton ChrisDenton added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 24, 2022
@ChrisDenton
Copy link
Member

It is currently fused in practice and I don't think there's a good reason to change that in the future. This would technically be an API change but one that should have no negative impact to existing code.

Which is the long winded way of saying I agree.

@AronParker
Copy link
Contributor Author

Thanks for the quick response! I realize that this is an API change but didn't feel it'd warrant a "feature suggestion". I guess for now I'm just throwing the idea out there.

@ChrisDenton
Copy link
Member

This is a great idea and I'd be happy to see a PR that implements it. I was just ticking off a checklist. It's not a breaking change and it is useful, so implementing it should not be at all controversial.

@AronParker
Copy link
Contributor Author

Sounds good, I'll get to it tomorrow!

@rustbot claim

@scottmcm
Copy link
Member

scottmcm commented Apr 25, 2022

Note that, procedurally, a new trait impl is insta-stable so will need a T-libs-api FCP.

(That's totally ok, just wanted to note it. You can and should send a PR for it.)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 27, 2022
Make EncodeWide implement FusedIterator

[`EncodeUtf16`](https://doc.rust-lang.org/std/str/struct.EncodeUtf16.html) and [`EncodeWide`](https://doc.rust-lang.org/std/os/windows/ffi/struct.EncodeWide.html) currently serve similar purposes: They convert from UTF-8 to UTF-16 and WTF-8 to WTF-16, respectively. `EncodeUtf16` wraps a &str, whereas `EncodeWide` wraps an &OsStr.

When Iteration has concluded, these iterators wrap an empty slice, which will forever yield `None` values. Hence, `EncodeUtf16` rightfully implements `FusedIterator`. However, `EncodeWide` in contrast does not, even though it serves an almost identical purpose.

This PR attempts to fix that issue. I consider this change minor and non-controversial, hence why I have not added a RFC/FCP. Please let me know if the stability attribute is wrong or contains a wrong version number. Thanks in advance.

Fixes rust-lang#96368
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 27, 2022
Make EncodeWide implement FusedIterator

[`EncodeUtf16`](https://doc.rust-lang.org/std/str/struct.EncodeUtf16.html) and [`EncodeWide`](https://doc.rust-lang.org/std/os/windows/ffi/struct.EncodeWide.html) currently serve similar purposes: They convert from UTF-8 to UTF-16 and WTF-8 to WTF-16, respectively. `EncodeUtf16` wraps a &str, whereas `EncodeWide` wraps an &OsStr.

When Iteration has concluded, these iterators wrap an empty slice, which will forever yield `None` values. Hence, `EncodeUtf16` rightfully implements `FusedIterator`. However, `EncodeWide` in contrast does not, even though it serves an almost identical purpose.

This PR attempts to fix that issue. I consider this change minor and non-controversial, hence why I have not added a RFC/FCP. Please let me know if the stability attribute is wrong or contains a wrong version number. Thanks in advance.

Fixes rust-lang#96368
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 28, 2022
Make EncodeWide implement FusedIterator

[`EncodeUtf16`](https://doc.rust-lang.org/std/str/struct.EncodeUtf16.html) and [`EncodeWide`](https://doc.rust-lang.org/std/os/windows/ffi/struct.EncodeWide.html) currently serve similar purposes: They convert from UTF-8 to UTF-16 and WTF-8 to WTF-16, respectively. `EncodeUtf16` wraps a &str, whereas `EncodeWide` wraps an &OsStr.

When Iteration has concluded, these iterators wrap an empty slice, which will forever yield `None` values. Hence, `EncodeUtf16` rightfully implements `FusedIterator`. However, `EncodeWide` in contrast does not, even though it serves an almost identical purpose.

This PR attempts to fix that issue. I consider this change minor and non-controversial, hence why I have not added a RFC/FCP. Please let me know if the stability attribute is wrong or contains a wrong version number. Thanks in advance.

Fixes rust-lang#96368
@bors bors closed this as completed in c4dd0d3 Apr 28, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Make EncodeWide implement FusedIterator

[`EncodeUtf16`](https://doc.rust-lang.org/std/str/struct.EncodeUtf16.html) and [`EncodeWide`](https://doc.rust-lang.org/std/os/windows/ffi/struct.EncodeWide.html) currently serve similar purposes: They convert from UTF-8 to UTF-16 and WTF-8 to WTF-16, respectively. `EncodeUtf16` wraps a &str, whereas `EncodeWide` wraps an &OsStr.

When Iteration has concluded, these iterators wrap an empty slice, which will forever yield `None` values. Hence, `EncodeUtf16` rightfully implements `FusedIterator`. However, `EncodeWide` in contrast does not, even though it serves an almost identical purpose.

This PR attempts to fix that issue. I consider this change minor and non-controversial, hence why I have not added a RFC/FCP. Please let me know if the stability attribute is wrong or contains a wrong version number. Thanks in advance.

Fixes rust-lang/rust#96368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants