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

Shared context #461

Closed
wants to merge 3 commits into from
Closed

Shared context #461

wants to merge 3 commits into from

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Aug 27, 2023

Supersedes #459

Based on this comment it seems like there's interest in passing shared contexts around in the raw API, this PR extracts just that aspect from #459 in so that at least the context aspect of #456 will be accepted.

Motivation: Passing a context around is necessary to allow eq and hasher to share the same mutable environment, Both can't implement FnMut and capture the same environment, because those two captures would both need to have exclusive access to it which is disallowed by Rust's aliasing rules.

There's three levels of possibly increasing controversy in this PR, separated by three commits:

  • The first one introduces a context which is passed around through *_with_context methods, maintaining the same API in the process (eq is impl FnMut).
  • The second commit makes both functions impl Fn for the new methods, meaning if you want to use a mutable shared environment there you must use the &mut C context variable to pass it around.
  • The third commit makes the internals use function pointers, completely disallowing any context that is not explicitly passed through &mut C.

If there is interest in passing a context around like this, feel free to decide which one you want to keep, and which one to discard.

Thank you!

@Amanieu
Copy link
Member

Amanieu commented Aug 27, 2023

What I had in mind was actually much simpler. find_or_find_insert_slot is the only method which takes both hasher and eq callbacks. Therefore only that method needs to support contexts which can be shared by both callbacks.

When there is only one callback, the caller can easily borrow the context exclusively in the closure environment. We would only need to change the hasher callbacks to be FnMut instead of Fn, but that shouldn't cause any problems.

@udoprog
Copy link
Contributor Author

udoprog commented Aug 27, 2023

It's certainly possible, I opted not to to maintain a more coherent API (and was considering commenting on it). Consider how the caller would otherwise need to build multiple kinds of adapters (like these or these) when interacting with it.

From your comment I gather that might not be an overriding concern for you?

@JustForFun88
Copy link
Contributor

It would be nice if the functions were documented (preferably with examples). After all, it is impossible to understand what context is and how to use it either at first or second glance.

We have to study the implementation.

@Amanieu
Copy link
Member

Amanieu commented Aug 27, 2023

I would prefer keeping the API simple and only add the context variant for find_or_find_insert_slot. Otherwise there might be confusion if there are 2 ways of doing the same thing (context vs capturing in closure).

@udoprog
Copy link
Contributor Author

udoprog commented Aug 27, 2023

Ok, which of the three variants above do you want find_or_find_insert_slot_with_context to use?

@Amanieu
Copy link
Member

Amanieu commented Aug 27, 2023

I would say the first variant (keep the API the same, just add a context), except that all hasher: impl Fn() are changed to hasher: impl FnMut().

@bors
Copy link
Contributor

bors commented Sep 5, 2023

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

@udoprog
Copy link
Contributor Author

udoprog commented Sep 5, 2023

Closing now since things are moving and I don't have the time to do this.

@udoprog udoprog closed this Sep 5, 2023
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.

4 participants