Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

Make the crate `no_std` + `alloc` (rebased) #48

Closed
wants to merge 6 commits into from
Closed

Make the crate `no_std` + `alloc` (rebased) #48

wants to merge 6 commits into from

Conversation

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

This is effectively a squash + rebase of #44 on top of #47.

It squashes the commits so that it is easier to see what's going on. @cuviper's comments from #44 (review) still apply, as does the question about whether we are even willing to make this change (#44 (comment)). There's a little twitter straw poll here. To summarize:

The new L parameter is unfortunate, but I don't see how we get around it if we want to support no_std. I think the only question is "is it important is it for us to support no_std for this crate". If the answer is that it seems valuable, then I think this is how it has to be.

Thanks to @GarkGarcia for all the hard work on #44 -- that's the majority of this change.

Closes #44. Fixes #26.

@jonhoo jonhoo force-pushed the rebased-44 branch 2 times, most recently from 432dcfb to a3b1322 Jan 30, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #48 into master will increase coverage by 0.75%.
The diff coverage is 87.75%.

Impacted Files Coverage Δ
src/iter/mod.rs 92.5% <ø> (ø) ⬆️
src/iter/traverser.rs 95.83% <100%> (-0.07%) ⬇️
src/raw/mod.rs 93.33% <100%> (+8.63%) ⬆️
src/node.rs 91.2% <100%> (-7.53%) ⬇️
src/map_ref.rs 74.5% <62.5%> (+2.81%) ⬆️
src/map.rs 87.55% <82.35%> (+0.93%) ⬆️
@jonhoo jonhoo force-pushed the rebased-44 branch from a3b1322 to 28d66a3 Feb 1, 2020
@jonhoo

This comment has been minimized.

Copy link
Owner Author

jonhoo commented Feb 1, 2020

Pushed some changes now that I think makes this patch a bit more pleasant. There is still the L parameter, but it should be much less noticeable since most users should be interacting with the HashMap type alias, which does not have it.

@jonhoo

This comment has been minimized.

Copy link
Owner Author

jonhoo commented Feb 1, 2020

Not sure if this alleviates some of your concerns from #44 @cuviper?

@GarkGarcia

This comment has been minimized.

Copy link
Contributor

GarkGarcia commented Feb 2, 2020

The new L parameter is unfortunate, but I don't see how we get around it if we want to support no_std. I think the only question is "is it important is it for us to support no_std for this crate". If the answer is that it seems valuable, then I think this is how it has to be.

Can't we also provide a default to the L parameter? That would alleviate some of the pain. No idea what that default would be in no_std though.

@jonhoo

This comment has been minimized.

Copy link
Owner Author

jonhoo commented Feb 2, 2020

Yeah, that's the problem -- on no_std, that parameter has no reasonable default. That in turn means that it cannot be the last type parameter (since you cannot have a type parameter without a default after one with a default). Which again in turn means that anywhere people specify map types, if they want to use a custom hasher, they have to write HashMap<K, V, _, S>. Also, the extra generic type is real awkward for anyone who wants to be generic over a map, though that's something we won't completely get away from anyway.

@cuviper

This comment has been minimized.

Copy link
Collaborator

cuviper commented Feb 3, 2020

I guess the type alias helps hide the complexity, at the loss of available iterator types. I'm not sure if that's a net win -- I guess that's still driven by how desirable no_std is in the first place.

Did you manage to gather compelling use-cases? And can you prototype that this crate actually works in those cases? What L would you use?

@jonhoo

This comment has been minimized.

Copy link
Owner Author

jonhoo commented Feb 3, 2020

I agree, it's definitely still a cost.

As far as use-cases goes, the response was pretty meager. https://twitter.com/thequux/status/1223248563523723264 and https://twitter.com/thequux/status/1223248563523723264 were the primary ones.

My current take is that we should probably just close this, with a note that we'd be open to considering if someone has a compelling use-case they're willing to also commit some time to supporting.

@cuviper

This comment has been minimized.

Copy link
Collaborator

cuviper commented Feb 3, 2020

My current take is that we should probably just close this, with a note that we'd be open to considering if someone has a compelling use-case they're willing to also commit some time to supporting.

I think that makes sense.

jonhoo added 6 commits Jan 30, 2020
This is necessary since there is no particular lock implementation that
"makes sense" on `no_std`. This patch changes all types to be generic
over any lock type that implements `lock_api::RawMutex`, and defaults to
`parking_lot::RawMutex` on `std`.

Note that this patch forks the struct definition for `HashMap` into one
for `std` (which has a default for `L`) and one for `no_std` (which does
not). This is fine since any code written _without_ the `std` feature
_will_ compile with the `std` feature enabled (`indexmap` does the same;
the reverse is not true). We _could_ introduce an intermediate private
struct to avoid repeating the definition, but it would require writing
`self.inner` all over the place, which would be sad. This seemed
marginally cleaner.

Also, this diff makes me want implied bounds a lot:
rust-lang/rust#44491

This is essentially a rebase of adb58e0
and f9ad723 on top of the previous
commit + #47.
This is essentially 7b32a16 rebased on
top of #47.
On `no_std`, users are forced to use `HashMap::guard` anyway, since
`epoch::pin` isn't available, so this seems fine.
This helps hide the `L` parameter even more. Unfortunately, I had to
remove `IntoIterator` for `&ConcurrentHashMapRef` since `impl Trait`
isn't supported in `type` position on trait impls yet.
@jonhoo jonhoo force-pushed the rebased-44 branch from d656367 to 250ac87 Feb 3, 2020
@jonhoo

This comment has been minimized.

Copy link
Owner Author

jonhoo commented Feb 3, 2020

All right, I'll close this and the corresponding issue then :)

@jonhoo jonhoo closed this Feb 3, 2020
@jonhoo jonhoo mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.