-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Align ArrayWindows trait impls with Windows
#151613
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
Conversation
The derived `T: Copy` constraint is not appropriate for an iterator by reference, but we generally do not want `Copy` on iterators anyway.
This implementation doesn't need the derived `T: Clone`.
|
The minimal fix would seem to be to backport an unstabling PR, then let these changes ride the train as normal. |
|
Given the number of missed things, it seems clear that this feature did not receive enough attention before it was stabilized. That's why I think unstabilizing is the most principled solution. Beta is the least used/tested release channel so backporting like this feels awfully close to committing straight to stable ;P. These changes seem straightforward enough, but the changes that end up causing problems often seem so as well... |
|
r? Amanieu |
|
@bors r+ |
…anieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. rust-lang#21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
…anieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. rust-lang#21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
…anieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. rust-lang#21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
…anieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. rust-lang#21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
…anieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. rust-lang#21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
Rollup of 12 pull requests Successful merges: - #152388 (`rust-analyzer` subtree update) - #151613 (Align `ArrayWindows` trait impls with `Windows`) - #152134 (Set crt_static_allow_dylibs to true for Emscripten target) - #152166 (cleanup some more things in `proc_macro::bridge`) - #152236 (compiletest: `-Zunstable-options` for json targets) - #152287 (Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait not implemented) - #142957 (std: introduce path normalize methods at top of `std::path`) - #145504 (Add some conversion trait impls) - #152131 (Port rustc_no_implicit_bounds attribute to parser.) - #152315 (fix: rhs_span to rhs_span_new) - #152327 (Check stalled coroutine obligations eagerly) - #152377 (Rename the query system's `JobOwner` to `ActiveJobGuard`, and include `key_hash`)
Rollup of 12 pull requests Successful merges: - #152388 (`rust-analyzer` subtree update) - #151613 (Align `ArrayWindows` trait impls with `Windows`) - #152134 (Set crt_static_allow_dylibs to true for Emscripten target) - #152166 (cleanup some more things in `proc_macro::bridge`) - #152236 (compiletest: `-Zunstable-options` for json targets) - #152287 (Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait not implemented) - #142957 (std: introduce path normalize methods at top of `std::path`) - #145504 (Add some conversion trait impls) - #152131 (Port rustc_no_implicit_bounds attribute to parser.) - #152315 (fix: rhs_span to rhs_span_new) - #152327 (Check stalled coroutine obligations eagerly) - #152377 (Rename the query system's `JobOwner` to `ActiveJobGuard`, and include `key_hash`)
Rollup of 12 pull requests Successful merges: - #152388 (`rust-analyzer` subtree update) - #151613 (Align `ArrayWindows` trait impls with `Windows`) - #152134 (Set crt_static_allow_dylibs to true for Emscripten target) - #152166 (cleanup some more things in `proc_macro::bridge`) - #152236 (compiletest: `-Zunstable-options` for json targets) - #152287 (Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait not implemented) - #142957 (std: introduce path normalize methods at top of `std::path`) - #145504 (Add some conversion trait impls) - #152131 (Port rustc_no_implicit_bounds attribute to parser.) - #152315 (fix: rhs_span to rhs_span_new) - #152327 (Check stalled coroutine obligations eagerly) - #152377 (Rename the query system's `JobOwner` to `ActiveJobGuard`, and include `key_hash`)
Rollup merge of #151613 - cuviper:array-windows-parity, r=Amanieu Align `ArrayWindows` trait impls with `Windows` With `slice::ArrayWindows` getting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared to `slice::Windows`, and I think we should align these. - Remove `derive(Copy)` -- we generally don't want `Copy` for iterators at all, as this is seen as a footgun (e.g. #21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiring `T: Copy`. - Manually `impl Clone`, simply to avoid requiring `T: Clone`. - `impl FusedIterator`, because it is trivially so. The `since = "1.94.0"` assumes we'll backport this, otherwise we should change that to the "current" placeholder. - `impl TrustedLen`, because we can trust our implementation. - `impl TrustedRandomAccess`, because the required `__iterator_get_unchecked` method is straightforward. r? libs-api @rustbot label beta-nominated (at least for the `Copy` removal, but we could be more selective about the rest).
With
slice::ArrayWindowsgetting ready to stabilize in 1.94, I noticed that it currently has some differences in trait implementations compared toslice::Windows, and I think we should align these.derive(Copy)-- we generally don't wantCopyfor iterators at all, as this is seen as a footgun (e.g. remove Copy impls from remaining iterators #21809). This is obviously a breaking change though, so we should only remove this if we also backport the removal before it's stable. Otherwise, it should at least be replaced by a manual impl without requiringT: Copy.impl Clone, simply to avoid requiringT: Clone.impl FusedIterator, because it is trivially so. Thesince = "1.94.0"assumes we'll backport this, otherwise we should change that to the "current" placeholder.impl TrustedLen, because we can trust our implementation.impl TrustedRandomAccess, because the required__iterator_get_uncheckedmethod is straightforward.r? libs-api
@rustbot label beta-nominated
(at least for the
Copyremoval, but we could be more selective about the rest).