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

feat(Pool): unify guard types #33

Closed
wants to merge 1 commit into from
Closed

Conversation

bIgBV
Copy link
Collaborator

@bIgBV bIgBV commented Apr 1, 2020

Begin unification of guard types by using common code paths for
clearing an entry.

Closes #30

* Begin unification of guard types by using common code paths for
clearing an entry
@bIgBV bIgBV added this to the Release-Ready Object Pool milestone Apr 1, 2020
@bIgBV
Copy link
Collaborator Author

bIgBV commented Apr 1, 2020

Hitting a problem with how the the Guard type is defined. Currently Guard is generic over T, but the compiler isn't happy about constructing a guard where the shard type is Option<T>.

sharded-slab/src/lib.rs

Lines 423 to 430 in c66204d

let shard = self.shards.get(tid.as_usize())?;
let inner = shard.get(key, |x| {
x.as_ref().expect(
"if a slot can be accessed at the current generation, its value must be `Some`",
)
})?;
Some(Guard { inner, shard, key })

It errors out with the following error message:

error[E0308]: mismatched types
   --> src/lib.rs:430:29
    |
240 | impl<T, C: cfg::Config> Slab<T, C> {
    |      - this type parameter
...
430 |         Some(Guard { inner, shard, key })
    |                             ^^^^^ expected type parameter `T`, found enum `std::option::Option`
    |
    = note: expected reference `&shard::Shard<T, C>`
               found reference `&shard::Shard<std::option::Option<T>, C>`
    = help: type parameters must be constrained to match other types
    = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters

Plus there are errors with the Drop impl due to the trait bounds not being met.

I might have an idea of how to fix it though. It should be possible to generic from Option to in shard.

@hawkw
Copy link
Owner

hawkw commented Apr 1, 2020

Hitting a problem with how the the Guard type is defined. Currently Guard is generic over T, but the compiler isn't happy about constructing a guard where the shard type is Option<T>.

Hmm. Ah, yeah, that's a roadblock I didn't think of. My guess is this may not really work the way we want it to. Maybe we should just continue having a separate pool guard type.

But, we can probably still have both guards use mark_clear rather than remove, which reduces some duplication. As that's an internal change, it wouldn't be a release blocker.

It would be nice to figure out whether or not we'll have to have two guard types or not, to stop holding up the release. But, I guess, this is 0.0.x, so we can ship now and break it if we figure out a better solution later.

@hawkw hawkw removed this from the Release-Ready Object Pool milestone Apr 2, 2020
@bIgBV
Copy link
Collaborator Author

bIgBV commented Apr 4, 2020

Closing this PR for now. I'll create a new one which includes the changes in this PR to unify the clear code path.

@bIgBV bIgBV closed this Apr 4, 2020
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.

Unify Guard and PoolGuard types
2 participants