Skip to content

Conversation

gmorenz
Copy link

@gmorenz gmorenz commented May 30, 2025

I didn't feel like making a custom pair struct for the ten lines of code I was using this map in and decided to try and implement BiHashItem for tuples. Somewhat surprisingly this appears to work fine. All the lifetime annotations appear to be necessary, though I would be lying if I said I fully understood why... they were all recommended by rustc.

There's an unfortunate naming clash here, where entity_map.get1(&square).unwrap().1 means using the first element of the tuple (get1) to get the second element of the tuple (.1). Renaming get1 and friends would solve this if that's something you would consider.

@sunshowers
Copy link
Collaborator

Thanks. This is an interesting idea, though I'd really like to push people towards using structs if possible. In practice I've found that tuples often get unwieldy to deal with and I'm worried supporting this for tuples encourages suboptimal code patterns. For example, adding non-key data, one of the core use cases for these maps, would require moving away from tuples, which in turn could require a potentially big refactoring.

Renaming get1 and friends would solve this if that's something you would consider.

This seems unfortunately, and I definitely would like to preserve the 1-indexing here -- to me it feels weird to number keys from 0.

@gmorenz
Copy link
Author

gmorenz commented May 30, 2025

Fair enough, I've got two short detailed responses, but I doubt they'll change your (entirely reasonable) opinion. Feel free to close this assuming they don't.

For example, adding non-key data, one of the core use cases for these maps, would require moving away from tuples, which in turn could require a potentially big refactoring.

An option here would be to support (K1, K2, A, B, C, ...) tuples where you can only index on the first 2/3 keys but can store longer ones.

This seems unfortunately, and I definitely would like to preserve the 1-indexing here -- to me it feels weird to number keys from 0.

A compromise solution would be first/second/third (with or without a get_ prefix... not sure which I prefer), but at the same time I entirely understanding preferring getN over those names.

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.

2 participants