Move MiniSet to data_structures

[andjo403]

remove the need for T to be copy from MiniSet as was done for MiniMap

MiniMap and MiniSet was added by #72412

think that this can be used in #68828

[rust-highfive]

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

[andjo403]

@VFLashM I was unable to find a reason for the MiniSet to require clone when MiniMap did not but maybe I missed something

[VFLashM]

There is no good reason other that MiniSet is only used with copyable types and that requiring copyable makes code a bit simpler. It's easy to remove this requirement just like it was done in MiniMap. If it's becoming a more generic piece of code, then it's absolutely is a good idea.

…

On Mon, Sep 21, 2020, 4:02 PM Andreas Jonson ***@***.***> wrote: @VFLashM <https://github.com/VFLashM> I was unable to find a reason for the MiniSet to require clone when MiniMap did not but maybe I missed something — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77028 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4NETN5JCNIBMLFBVEV5G3SG65PXANCNFSM4RU7S7LQ> .

[jyn514]

Might be pre-mature generalization, but I think you could make this generic over the length of the array too.

Suggested change

	pub enum MiniMap<K, V> {
	Array(ArrayVec<[(K, V); 8]>),
	pub enum MiniMap<K, V, const N = 8> {
	Array(ArrayVec<[(K, V); N]>),

[andjo403]

I hade tried but I was not able to get it to work with how arrayvec is implemented.
I get this fault:

error[E0277]: the trait bound `[T; N]: arrayvec::Array` is not satisfied in `arrayvec::ArrayVec<[T; N]>`
  --> compiler/rustc_data_structures/src/mini_set.rs:10:11
   |
10 |     Array(ArrayVec<Array<T, N>>),
   |           ^^^^^^^^^^^^^^^^^^^^^ within `arrayvec::ArrayVec<[T; N]>`, the trait `arrayvec::Array` is not implemented for `[T; N]`
   |
   = help: the following implementations were found:
             <[T; 0] as arrayvec::Array>
             <[T; 100] as arrayvec::Array>
             <[T; 1024] as arrayvec::Array>
             <[T; 10] as arrayvec::Array>
           and 53 others
   = note: required because it appears within the type `arrayvec::ArrayVec<[T; N]>`
   = note: no field of an enum variant may have a dynamically sized type
   = help: change the field's type to have a statically known size
help: borrowed types always have a statically known size
   |
10 |     Array(&ArrayVec<Array<T, N>>),
   |           ^
help: the `Box` type always has a statically known size and allocates its contents in the heap
   |
10 |     Array(Box<ArrayVec<Array<T, N>>>),
   |           ^^^^                     ^

[VFLashM]

I know it is possible for Set, it just has to be changed to accept single type argument - array, just like arrayvec does.
I'm not sure if it's realistically possible for map, because then you'll need a way to constrain array to be an array of pairs and then some way to extract key and value types from said pair. Not sure if it's possible without an additional bunch of hacky hardcoded traits.

Either way both solutions are quite cumbersome and have no real use as of now.

[jyn514]

Oh haha it doesn't work because arrayvec itself hard coded some impls and didn't use const generics.

That's fine then, no need to add it. I just thought it would be nice.

[VFLashM]

On a side note I'm working on a huge change adding vast amount of missing API from HashMap/HashSet so MiniSet/MiniMap can be used across the board.
It is based on top of this PR.
Just a heads up if anyone needed it.

[bors]

☔ The latest upstream changes (presumably #76928) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[andjo403]

have rebased and MiniMap was already moved by #76928 so this will now only move MiniSet as I was not able to see any PR that move that

[matthewjasper]

@bors r+

[bors]

📌 Commit 6586c37 has been approved by matthewjasper

[bors]

⌛ Testing commit 6586c37 with merge 86b4172...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: matthewjasper
Pushing 86b4172 to master...