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

Implement no_std support using hashbrown and lock_api #9

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juntyr
Copy link
Contributor

@juntyr juntyr commented Feb 5, 2023

Follow-up to #6

This second attempt is a shot at a very different design that attempts to keep the feature matrix as slim and non-annoying as possible. It is breaking since it makes hashbrown non-optional and removes the parking_lot feature (since the Interner is now always parameterised by a lock_api::RawRwLock, so parking_lot can be supported by any user).

To get std support working as-is, the Interner uses type defaults for the hash builder and rwlock with the std feature. For this, I've implemented a non-trivial private wrapper around std::sync::Rwlock.

If this PR would be the direction to go, I'd still need to write a nice change log entry to document the breaking change. If not, it can hopefully still serve as inspiration for how to implement no_std support.

@juntyr juntyr changed the title Implement no-std support using hashbrown and lock_api Implement no_std support using hashbrown and lock_api Feb 5, 2023
@CAD97
Copy link
Owner

CAD97 commented Feb 6, 2023

Just to note, I don't have much spare time to spend on OSS atm (final semester at $gradschool!), so I'll probably not get to this soon; it's a significant chunk of API/impl to digest.

Feel free to yoink what you need in the meantime; the project is intended to be trivial enough that it could be vendored without issue.

Comment on lines +88 to +92
// Safety:
// - unlock_shared may only be called if a shared lock is held
// in the current context
// - thus an old guard for that shared lock must exist
let old_guard = std::ptr::read(&new_guard);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very unconvinced that this is guaranteed to work as intended. I agree that it does work without UB for the current std distribution, but this isn't guaranteed to be the case forever, e.g. if RwLockReadGuard owns some allocation in the future rather than just borrowing the RwLock.

I'd be more comfortable if lock_api provided a raw version of the std lock for use with lock_api. That they don't optionally provide such suggests that they agree with me that this isn't sound.

Comment on lines 16 to +23
[features]
raw = ["hashbrown", "hashbrown/raw"]
default = ["std"]
std = []
raw = ["hashbrown/raw"]

[dependencies]
parking_lot = { version = "0.12.1", optional = true, default-features = false }
hashbrown = { version = "0.13.2", optional = true, default-features = false }
hashbrown = { version = "0.13.2", default-features = false }
lock_api = { version = "0.4.9" }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make this not API-breaking by adding dummy features for the no-longer-optional crates which don't do anything.

(I haven't inspected the rest of the touched API surface.)

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.

None yet

2 participants