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 <[[T; N]]>::flatten{_mut} #95579

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Conversation

Cyborus04
Copy link
Contributor

Adds flatten to convert &[[T; N]] to &[T] (and flatten_mut for &mut [[T; N]] to &mut [T])

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 2, 2022
@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 @yaahc (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 2, 2022
@scottmcm
Copy link
Member

scottmcm commented Apr 2, 2022

Since it's now (#95016) agreed to be sound, consider adding Vec::flatten to go with these as well.

(Or maybe it should be into_flattened or something to not shadow the slice method.)

@rust-log-analyzer

This comment has been minimized.

@Cyborus04
Copy link
Contributor Author

r? @scottmcm

@rust-log-analyzer

This comment has been minimized.

@rust-highfive rust-highfive assigned scottmcm and unassigned yaahc Apr 2, 2022
@rust-log-analyzer

This comment has been minimized.

@Cyborus04
Copy link
Contributor Author

Cyborus04 commented Apr 2, 2022

Wow, is there really no usage of unchecked_* math in alloc? That can't be right...

@scottmcm
Copy link
Member

scottmcm commented Apr 2, 2022

Wow, is there really no usage of unchecked_* math in alloc? That can't be right...

It doesn't surprise me, really. As the feature explanation says, it's a niche optimization path where it's extremely rare for it to be actually relevant. And the building blocks that are foundational enough to use it tend to be in core.

@Cyborus04
Copy link
Contributor Author

Oh, I missed that before I pushed that commit. Should I just used normal * multiplication then?

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

scottmcm commented Apr 2, 2022

Oh, I missed that before I pushed that commit. Should I just used normal * multiplication then?

Naw, it's fine. It's plausibly helpful for optimization, since otherwise LLVM doesn't know that the slice didn't get shorter.

@leonardo-m
Copy link

Another use case is to convert &[[T; N]; M] to &[T; N * M]. That's a case of array reshaping in general.

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. 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 Apr 2, 2022
@rust-log-analyzer

This comment has been minimized.

@Cyborus04
Copy link
Contributor Author

🤦🏻‍♂️

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor Author

@Cyborus04 Cyborus04 left a comment

Choose a reason for hiding this comment

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

oh uh messed this one up a bit

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2022

As far as I know we don't want to use anything that's still incomplete_features in the bootstrap compiler. Unstable is fine when it's decently-baked, but if compiler says it's incomplete, we don't use it in libs.


I found a few more nits, and I have one extra request: Can you add #[should_panic(expect = "... tests for the overflow cases, please?

So something like

#[test]
#[should_panic(expect = "vec len overflow")]
fn vec_into_flattened_panics_for_capacity_overflow() {
    let n = 1 << (usize::BITS / 2);
    let _ = vec![[(); n]; n].into_flattened();
}

which would probably go in https://github.com/rust-lang/rust/blob/master/library/alloc/tests/vec.rs

And analogous ones for the two slice methods.

When you're done, please comment @rustbot ready to update the tags accordingly.

@rust-log-analyzer

This comment has been minimized.

@Cyborus04
Copy link
Contributor Author

Cyborus04 commented Apr 6, 2022

wait i put it in the wrong file 😅

@Cyborus04
Copy link
Contributor Author

What do I do about the feature flag issue? There's other tests that use unstable features without any #[feature(...)] attributes

@scottmcm
Copy link
Member

scottmcm commented Apr 6, 2022

I think you need to allow the new feature in the tests library, like all of these ones

#![feature(allocator_api)]
#![feature(alloc_layout_extra)]
#![feature(assert_matches)]
#![feature(box_syntax)]
#![feature(cow_is_borrowed)]
#![feature(const_box)]
#![feature(const_convert)]
#![feature(const_cow_is_borrowed)]
#![feature(const_heap)]
#![feature(const_intrinsic_copy)]
#![feature(const_mut_refs)]
#![feature(const_nonnull_slice_from_raw_parts)]
#![feature(const_ptr_write)]
#![feature(const_try)]
#![feature(core_intrinsics)]
#![feature(drain_filter)]
#![feature(exact_size_is_empty)]
#![feature(new_uninit)]
#![feature(pattern)]
#![feature(trusted_len)]
#![feature(try_reserve_kind)]
#![feature(unboxed_closures)]
#![feature(associated_type_bounds)]
#![feature(binary_heap_into_iter_sorted)]
#![feature(binary_heap_drain_sorted)]
#![feature(slice_ptr_get)]
#![feature(binary_heap_retain)]
#![feature(binary_heap_as_slice)]
#![feature(inplace_iteration)]
#![feature(iter_advance_by)]
#![feature(round_char_boundary)]
#![feature(slice_group_by)]
#![feature(slice_partition_dedup)]
#![feature(string_remove_matches)]
#![feature(const_btree_new)]
#![feature(const_default_impls)]
#![feature(const_trait_impl)]
#![feature(const_str_from_utf8)]
#![feature(nonnull_slice_from_raw_parts)]
#![feature(panic_update_hook)]

@Cyborus04 Cyborus04 force-pushed the slice_flatten branch 2 times, most recently from aabf8d1 to 405aaba Compare April 7, 2022 02:00
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Let's see if it'll just let me commit these suggestions...

library/core/tests/slice.rs Show resolved Hide resolved
library/core/tests/slice.rs Show resolved Hide resolved
@Cyborus04
Copy link
Contributor Author

Ohhh, thank you! I've been confused about this all day, glad it's finally solved 😀

@Cyborus04
Copy link
Contributor Author

Cyborus04 commented Apr 8, 2022

uh oh i think i destroyed your commit by accident, sorry. forgot to pull first

@scottmcm
Copy link
Member

scottmcm commented Apr 8, 2022

Looks good! Thanks for your perseverance through all the hiccups.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 8, 2022

📌 Commit 06788fd has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 8, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#95102 (Add known-bug for rust-lang#95034)
 - rust-lang#95579 (Add `<[[T; N]]>::flatten{_mut}`)
 - rust-lang#95634 (Mailmap update)
 - rust-lang#95705 (Promote x86_64-unknown-none target to Tier 2 and distribute build artifacts)
 - rust-lang#95761 (Kickstart the inner usage of `macro_metavar_expr`)
 - rust-lang#95782 (Windows: Increase a pipe's buffer capacity to 64kb)
 - rust-lang#95791 (hide an #[allow] directive from the Arc::new_cyclic doc example)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d5232c6 into rust-lang:master Apr 8, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 8, 2022
@scottmcm
Copy link
Member

scottmcm commented Apr 8, 2022

Congrats on your first PR landing, @Cyborus04!

@robjtede robjtede mentioned this pull request Apr 25, 2022
65 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2022
…, r=Mark-Simulacrum

Add a codegen test for `slice::from_ptr_range`

I noticed back in rust-lang#95579 that this didn't optimize as well as it should.

It's better now, after rust-lang#95837 changed the code in `from_ptr_range` and llvm/llvm-project#54824 was fixed in LLVM 15.

So here's a test to keep it generating the good version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants