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

replace next_id with alloc_with_id #9

Closed
matklad opened this issue Jan 29, 2019 · 3 comments
Closed

replace next_id with alloc_with_id #9

matklad opened this issue Jan 29, 2019 · 3 comments

Comments

@matklad
Copy link
Collaborator

matklad commented Jan 29, 2019

fn alloc_with_id(f: impl FnOnce(ID) -> T)

seems like potentially more bullet-proof API?

@fitzgen
Copy link
Owner

fitzgen commented Jan 29, 2019

Unless there is also an assertion against re-entrancy, I don't think this actually removes potential bugs resulting from operation orderings like

  1. get next id (either via next_id or alloc_with_id)
  2. "unexpected" alloc
  3. alloc the thing trying to use the id gotten in (1)

I'm not convinced it is worth adding the overhead that an assertion against re-entrancy would require...

@matklad
Copy link
Collaborator Author

matklad commented Jan 29, 2019

alloc takes &mut, so no reentrancy is possible

@fitzgen
Copy link
Owner

fitzgen commented Jan 30, 2019

Ah! great point! Yeah, I'd be in favor of this then!

fitzgen added a commit that referenced this issue Jan 30, 2019
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

No branches or pull requests

2 participants