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

RawTable allocator can't be used by stable code #226

Closed
cuviper opened this issue Jan 18, 2021 · 4 comments · Fixed by #227
Closed

RawTable allocator can't be used by stable code #226

cuviper opened this issue Jan 18, 2021 · 4 comments · Fixed by #227

Comments

@cuviper
Copy link
Member

cuviper commented Jan 18, 2021

I see that #133 tried to make RawTable's allocator parameter safe for stable code:

  • RawTable has a new type parameter, A: Alloc + Clone
  • When the nightly flag is passed, Alloc will be the trait in alloc::Alloc.
  • On stable, a minimal shim implementation is provided, along with an implementation for the global allocator.

However, I don't think this is sufficient as-is. There is an unfortunate side effect that crate features can be enabled indirectly, forcing the unstable feature on crates that aren't prepared for it. For instance:

  • Suppose I update indexmap to hashbrown 0.10 with just the "raw" feature, not "nightly". I'll use the stable shim version of hashbrown::raw::Global, and it seems fine.
  • Something else in a user's dependency graph enables hashbrown "nightly". It should be fine if the user is opting into the nightly compiler for their project.
  • indexmap now fails to compile for lack of #![feature(allocator_api)], since Global became the unstable type from std.

I think this might have been avoided if RawTable had a default A = Global, so indexmap wouldn't have to name it at all. It also seems problematic that with_capacity needs an allocator parameter, although Default::default() seems to work ok. I would have expected it to remain without that parameter though, perhaps with_capacity_in for that instead, like new/new_in.

Even with default A = Global, I think it's a problem that Global may or may not require #![feature(allocator_api)]. A stable user might explicitly choose to specify the "stable" Global, and still have the rug pulled out when "nightly" is enabled elsewhere. This is also true of HashMap and HashSet which do have A = Global now.

@cuviper
Copy link
Member Author

cuviper commented Jan 18, 2021

I think the safest bet would be to add A = Global, and then have the entire stable-shim implementation exist only as pub-in-private, not actually reachable outside the crate. This way stable users can only use an implicit Global, and nightly users must use the actual unstable paths from the standard library.

@Amanieu
Copy link
Member

Amanieu commented Jan 18, 2021

Several people have interest in using hashbrown with bumpalo, which was the original motivation for adding allocator support to hashbrown.

Perhaps this is better exposed by adding a "bumpalo" optional feature that specifically exports a BumpHashMap on both stable and nightly?

@Amanieu
Copy link
Member

Amanieu commented Jan 18, 2021

I've yanked 0.10.0 until this gets resolved.

@cuviper
Copy link
Member Author

cuviper commented Jan 18, 2021

I started working on my suggestion for defaults and hidden (pub-in-private) shims. I think that may be orthogonal to adding bumpalo-specific types.

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 a pull request may close this issue.

2 participants