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

Proposal: Change iterator implementations to return borrowed keys #47

Open
Themayu opened this issue Feb 27, 2024 · 4 comments
Open

Proposal: Change iterator implementations to return borrowed keys #47

Themayu opened this issue Feb 27, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@Themayu
Copy link

Themayu commented Feb 27, 2024

Or: "Do the iterators really need to copy the keys by default?"

I'm having a bit of trouble creating an implementation for bevy_reflect::Map on a wrapper type around your fixed_map::Map type. The problem stems from the requirement that bevy_reflect::Map::get_at and bevy_reflect::Map::get_at_mut return the key-value pairs as borrowed dyn Reflect trait objects. While this is easy for the values (since they are borrowed anyway), there comes a problem with the keys. Because the keys are given from your iter() and iter_mut() iterators as owned values, this makes it impossible to meet the requirement for the trait.

Due to how trivial it is to acquire an owned key if necessary for a user's use-case (since the keys must implement Copy, simply doing *key in the iterator adaptor is good enough), I propose that the iterator implementations instead return borrows of the keys and leave it to the user to copy them if necessary.

@udoprog udoprog added the enhancement New feature or request label Feb 27, 2024
@udoprog
Copy link
Owner

udoprog commented Feb 27, 2024

I don't mind. I can't remember why they're copy, so if you want to give it a stab, please document in case you come up against any problems.

@Themayu
Copy link
Author

Themayu commented Feb 28, 2024

Looking at the way fixed_map::Key gets expanded in both the simple (unit variants) and complex (composite keys) cases, there's a problem that needs to be resolved in that currently, keys aren't stored at all in the MapStorage. This presents a problem because it means there's nowhere for the keys to be borrowed from.

In the simple case, we could store the list of keys somewhere in the storage to get around this, with the trade-off of making __MapStorage larger as a result. In the complex case, I'm not sure what we would do.

EDIT: Oops. Pressed send slightly too early.

@udoprog
Copy link
Owner

udoprog commented Feb 28, 2024

Ah yeah. That's clearly also an issue for SetStorage, since for simple enums it's implemented as a boolean array. Or as a bitset if #[key(bitset)] is specified.

Tricky! I'm not sure how to construct compound keys like for OptionStorage without resorting to more macros 🤔

@Themayu
Copy link
Author

Themayu commented Feb 28, 2024

For compound keys I was thinking of delegating the key storage to the innermost layer that each variant combination can reach. Though with long chains, that can risk massively bloating up the storage space of the innermost layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants