3

Add `<[[T; N]]>::flatten{_mut}` by Cyborus04 · Pull Request #95579 · rust-...

 2 years ago
source link: https://github.com/rust-lang/rust/pull/95579
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

New issue

Add <[[T; N]]>::flatten{_mut} #95579

Conversation

Copy link

Contributor

@Cyborus04 Cyborus04 commented 16 days ago

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

ChayimFriedman2 and GrayJack reacted with hooray emoji

rustbot

added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label

16 days ago

Copy link

Collaborator

rust-highfive commented 16 days ago

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

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

16 days ago

Copy link

Member

scottmcm commented 16 days ago

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

This comment has been minimized.

Copy link

Contributor

Author

Cyborus04 commented 16 days ago

This comment has been minimized.

This comment has been minimized.

Copy link

Contributor

Author

Cyborus04 commented 16 days ago

edited

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

Copy link

Member

scottmcm commented 16 days ago

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.

Copy link

Contributor

Author

Cyborus04 commented 16 days ago

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

This comment has been minimized.

Copy link

Member

scottmcm commented 15 days ago

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.

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

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

15 days ago

This comment has been minimized.

Copy link

Contributor

Author

Cyborus04 commented 15 days ago

🤦🏻‍♂️

This comment has been minimized.

This comment has been minimized.

Copy link

Contributor

Author

@Cyborus04 Cyborus04 left a comment

oh uh messed this one up a bit

Copy link

Contributor

Author

Cyborus04 commented 12 days ago

That works, but is it okay to so directly expose an unstable feature?

Copy link

Member

scottmcm commented 12 days ago

edited

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.

This comment has been minimized.

Copy link

Contributor

Author

Cyborus04 commented 11 days ago

edited

wait i put it in the wrong file sweat_smile

scottmcm reacted with laugh emoji

Copy link

Contributor

Author

Cyborus04 commented 11 days ago

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

Copy link

Member

scottmcm commented 11 days ago

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)]

This comment has been minimized.

This comment has been minimized.

Copy link

Member

@scottmcm scottmcm left a comment

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

Cyborus04 reacted with thumbs up emoji

Copy link

Contributor

Author

Cyborus04 commented 10 days ago

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

scottmcm reacted with heart emoji

Copy link

Contributor

Author

Cyborus04 commented 9 days ago

edited

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

Copy link

Member

scottmcm commented 9 days ago

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

@bors r+

Cyborus04 reacted with thumbs up emoji

Copy link

Contributor

bors commented 9 days ago

pushpin Commit 06788fd has been approved by scottmcm

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

9 days ago

bors

merged commit d5232c6 into

rust-lang:master 9 days ago

10 checks passed

rustbot

added this to the 1.62.0 milestone

9 days ago

Copy link

Member

scottmcm commented 9 days ago

Congrats on your first PR landing, @Cyborus04!

Cyborus04 reacted with hooray emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

scottmcm

Assignees

scottmcm

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

Milestone

1.62.0

Development

Successfully merging this pull request may close these issues.

None yet

8 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK