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 dynamically-sized slices of maps and sets #177

Merged
merged 16 commits into from
May 7, 2022

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Feb 28, 2021

Both map::Slice<K, V> and set::Slice<T> are true DSTs, simply wrapping [Bucket], created by reference either indexing with ranges or calling as_slice() or as_mut_slice(). They support many of the normal indexing methods of slices, like splitting, but nothing that would change the order or require hashing. They also reuse the iterator types from map and set. Slice equality does consider the order of the elements, and this is followed by PartialOrd, Ord, and Hash as well.

@cuviper
Copy link
Member Author

cuviper commented Feb 28, 2021

I need to add a bunch of tests, but the implementation is pretty direct, and I wanted to go ahead and share the concept.

This is sort of an evolution of the "views" idea in #47, and it also allows sliced iterators like #104.

@bluss
Copy link
Member

bluss commented May 6, 2021

This is very nice. Slices have such nice properties, so a lot can be done using this. I was unsure if Deref should be implemented, that creates a very strong link between the set of methods exposed in both types.

@cuviper
Copy link
Member Author

cuviper commented May 19, 2021

I was unsure if Deref should be implemented, that creates a very strong link between the set of methods exposed in both types.

I'm not sure either. There's a lot of duplication right now in indexing methods, and in a new semver we might let Deref handle that alone. But if this is contentious, I don't mind removing it.

I'm not sure about AsRef and AsMut either. Given that we guard the construction of a Slice, there's probably not much use for a generic way to do it, rather than using the inherent methods. Maybe some wrapper might want to implement the same AsRef to use in a generic setting, but it's a stretch.

@bluss
Copy link
Member

bluss commented May 21, 2021

I'd like to think of it as not contentious, more about a careful approach.. we don't know if it's a good idea with Deref, so we hold it off?

However, I can see that both choices Deref/no deref impact the methods provided, so it's not necessarily that simple.

Without looking at the details, I just suspect that there are methods/operators on Slice that will feel unintuitive and surprising, perhaps even misleading when applied on the IndexMap. That's why I'd like to hold off on the Deref impl.

Now, Index impls on Slice should not apply on IndexMap through deref (because IndexMap implements slice by itself), but that's the kind of thing that can be a bit surprising.

If we compare with Vec, the interface change (or "loss") because of the Vec -> slice conversion is relatively smaller than what we have here - both Vec and slice are integer-indexed sequences. A map slice is a whole another thing than a hash map, so it feels weird to have it implicit. If would be less weird, if it allowed map lookup!

Maybe AsRef can follow Deref. If we have Deref, then it makes sense to expect AsRef too.

@cuviper
Copy link
Member Author

cuviper commented May 21, 2021

Ok, I rebased and left out Deref and AsRef for now.

@cuviper
Copy link
Member Author

cuviper commented May 21, 2021

Additional ideas:

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

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

Seems great. If we can think of some basic tests to add, it could be indexing.

@cuviper
Copy link
Member Author

cuviper commented May 27, 2021

I've added a few more things, including some indexing tests.

@mwillsey
Copy link
Contributor

I love this idea! One question though: why not expose (and rename if needed) Bucket and just use &[Bucket]? A public API for Bucket seems less complex than that of Slice.

@cuviper
Copy link
Member Author

cuviper commented Sep 22, 2021

One reason is mutability -- we try not to expose key-changing abilities without at least opting in to the MutableKeys trait. (There's one mistake with get_index_mut which we missed before 1.0.) Here I can expose &mut Slice<K, V> that still protects keys, but &mut [Bucket<K, V>] would give you full reign to move elements around and break the hash relationship, even if Bucket doesn't expose key mutability.

Otherwise I think it's just a preference not to expose implementation details, but you're probably right that it wouldn't be too bad to provide a simple public API on that.

@mwillsey
Copy link
Contributor

Ah, good point about &mut [Bucket<K, V>]. It seems to me that it's a relatively small footgun though: it's safe (I think) and if Bucket isn't Clone, then you really have to mess up (using a slice method I guess) to move things around.

A even simpler idea would be to only expose &[Bucket]. To me, the utility of a mutable slice is pretty low, especially when there are other ways to mutate the data structure.

@cuviper
Copy link
Member Author

cuviper commented Sep 22, 2021

then you really have to mess up (using a slice method I guess) to move things around.

That's a lot of surface area to mess up, and there's no barrier to calling those methods. For example, reverse, sort, and swap would all be in error, but that may be surprising since we do allow those on the map itself.

@cuviper cuviper mentioned this pull request Feb 9, 2022
@cuviper
Copy link
Member Author

cuviper commented Apr 2, 2022

Another idea I've just added is into_boxed_slice(self) -> Box<Slice<..>>, and support for all the owned iterators from there.

@cuviper
Copy link
Member Author

cuviper commented May 7, 2022

I think there are no concerns here, so let's go!

@cuviper cuviper merged commit 5ba716b into indexmap-rs:master May 7, 2022
@cuviper cuviper deleted the slice branch July 18, 2023 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants