Skip to content
This repository has been archived by the owner on Sep 11, 2023. It is now read-only.

Expose the array field (or add as_array(&self) -> &K::Array) #26

Closed
Fuuzetsu opened this issue Aug 5, 2022 · 8 comments · Fixed by #29
Closed

Expose the array field (or add as_array(&self) -> &K::Array) #26

Fuuzetsu opened this issue Aug 5, 2022 · 8 comments · Fixed by #29

Comments

@Fuuzetsu
Copy link
Contributor

Fuuzetsu commented Aug 5, 2022

It's currently possible to:

  • Create EnumMap from K::Array with from_array
  • Destructure EnumMap to K::Array with into_array
  • Get a slice reference with as_slice and as_mut_slice

But it's currently not possible to get a reference to K::Array directly, mutable or not. This is useful if the contents aren't Clone-able but we still want to iterate over the values directly (without concerning ourselves with the keys).

At this point, it seems that just exposing the array field of EnumMap as public should be fair game: we already have setter/getter and hiding it just makes it inconvenient.

@khoover
Copy link

khoover commented Aug 29, 2022

I can agree that having access to the underlying array by ref is useful (in particular you can eliminate runtime bounds checking in some cases without unsafe), but I'm not sure how as_array would help for the iterating case (again, minus bounds checking). You either need the array by-value to iterate by-value, or you can use as_slice().iter() or as_mut_slice().iter_mut().

@Fuuzetsu
Copy link
Contributor Author

but I'm not sure how as_array would help for the iterating case (again, minus bounds checking).

It helps exactly for bounds checking. For example, we can take two enum maps &EnumMap<K, X> and &EnumMap<K, Y>, get arrays &[X; N] and &[Y; N], and have some function f(x: &X, y: &Y) -> Z. With this we can create a new EnumMap<K, Z> by some zip function without having to handle mismatching lengths case nor ever inspecting the keys nor taking ownership of initial maps. If you just have a slice, you have to add bogus error handling/panics which clutter the code at best.

FWIW this kind of zip as well as some other operations is what we're looking to do, we have a lot of same-key EnumMaps floating around and the existing methods have too much overhead for us as we don't usually care about what the key is, just about union of values and such.

@Fuuzetsu
Copy link
Contributor Author

I guess the confusion came from:

This is useful if the contents aren't Clone-able but we still want to iterate over the values directly (without concerning ourselves with the keys).

I wanted to say that we'll be able to iterate over values ignoring the keys: of course they'll be references to the values as you mention. We don't want a slice as that removes the array length which is the main thing we care about as we want to manipulate multiple EnumMap together and need them to align.

@khoover
Copy link

khoover commented Aug 29, 2022

If the maps you're zipping together all use the same enum for keys, you can try creating iterator types that go through each variant in turn; then your zipping becomes EnumIter::new().map(|key| { (mapA[key], mapB[key]) }). I believe that should still avoid the bounds checking.

@Fuuzetsu
Copy link
Contributor Author

Fuuzetsu commented Aug 29, 2022

You missed the "ignoring the keys" part: I don't want to see and use key because it's not free to convert to/from usize representation, especially for non-trivial enums. It is much cheaper (we measured) to instead take an existing map (or multiple maps) and operate on the values only if you don't need the key.

Having access to the arrays by reference allows us to uniformly access multiple EnumMaps safely (ensured at compile time) and efficiently (both because we don't have to do bounds checks but also because we don't need to ever manipulate the keys).

Your proposed solution requires key in order to index into the other maps which is exactly what we want to avoid.

@Fuuzetsu
Copy link
Contributor Author

Your proposed solution requires key in order to index into the other maps which is exactly what we want to avoid.

Heck, even if you do want a key, multiple operations can be more efficient. You can have some zip_with_key operation that only generates the keys (K -> usize) which is cheap and probably spirited away at compile time. It's the indexing that the key in your example is then used for which can be expensive (usize -> K – non-trivial, code has panic handlers, might be optimised away if inlined and the caller is simple enough but probably not).

@chrisbouchard
Copy link

chrisbouchard commented Sep 3, 2022

Where I would find this helpful is that right now, as far as I can tell, there's no way to map over an EnumMap without consuming the EnumMap if the original values aren't Clone and the result values aren't Default.

Here's my use-case: I have something like

#[derive(Debug, Default)]
struct Foo { ... };

#[derive(Debug)]
struct View<'a> {
    foo: &'a Foo,
}

impl Foo {
    pub fn view(&self) -> View<'_> {
        View { foo: self }
    }
}

let foo_map: EnumMap<Key, Foo> = EnumMap::default();
let view_map: EnumMap<Key, View> = /* ? */;

I can't say

let view_map = foo_map.clone().map(Foo::view);

because (a) that requires Foo: Clone, and (b) even if it were, I'd only get a reference to the temporary clone which will not live long enough. I also can't say

let view_map: EnumMap<_, _> = foo_map
    .iter()
    .map(|(id, foo)| (id, foo.view()))
    .collect();

because EnumMap<K, V>: FromIterator requires V: Default. Of course, as you pointed out in an earlier comment, I can use the enum_map macro,

let view_map = enum_map! {
    key => foo_map[key].view(),
}

which works, but why does the macro get to use magic that I can't replicate using simple iterators? 😄 (I know, it forces there to be a mapping for every key, so there's no chance of an entry being left uninitialized.) Also, it wouldn't help if I needed to anything more complicated than map or zip.

To do this today with just iterators, the only solution I could find is,

let view_array: <Key as EnumArray<_>>::Array = foo_map
    .values()
    .map(Foo::view)
    .collect<Vec<_>>()  // Can't collect directly into an array.
    .try_into()
    .expect("EnumMap gave us a slice with a different length");
let view_map = EnumMap::from_array(view_array);

which requires a heap allocation and unwrapping a Result to convert from Vec<View> to [View; _].

But, if I had EnumMap::as_array (and didn't mind using nightly for array_methods) I could just say:

let view_array = foo_map.as_array().each_ref().map(Foo::view);
let view_map = EnumMap::from_array(view_array);

Since the array is bounded, I could manipulate it safely and give it back to EnumMap without worry.

Having EnumMap<K, V>: From<<K as EnumArray<V>>::Array> too would be even nicer — then this would even be a reasonable one-liner.

let view_map: EnumMap<_, _> = foo_map.as_array().each_ref().map(Foo::view).into();

Of course, another solution would be to have EnumMap's FromIterator implementation use the same magic that the enum_map is using with MaybeUninit so that it doesn't require V: Default. But then we'd I think we'd need to have a fallible FromIterator, e.g., for Result<EnumMap<K, V>, UnmappedKeyError>.

Fuuzetsu added a commit to Fuuzetsu/enum-map that referenced this issue Oct 5, 2022
An alternative would be to simply expose the `array` field as `pub`:
whatever works. I went with this as it allows adding simple docs at the
same time.

Fixes KamilaBorowska#26.
Fuuzetsu added a commit to Fuuzetsu/enum-map that referenced this issue Oct 5, 2022
An alternative would be to simply expose the `array` field as `pub`:
whatever works. I went with this as it allows adding simple docs at the
same time.

Fixes KamilaBorowska#26.
@chrisbouchard
Copy link

Very exciting!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants