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: add return val to replace_bucket_with replace_entry_with #419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Daniel-Aaron-Bloom
Copy link

If a user has a RawOccupiedEntryMut, they should be able to use replace_entry_with to conditionally remove it. However, if a user wishes to extract data from this entity during the removal process, the current api necessitates the use of unwrap:

/// Extract the coolness of an item, possibly destroying it
fn extract_coolness<V>(v: V) -> (Option<V>, Coolness) {
    /// ...
}

let entry: RawOccupiedEntryMut< _, _, _, _> = unimplemented!();

let mut coolness = None;
let entry = entry.replace_entry_with(|_k, v| {
    let (maybe_v, rcool) = extract_coolness(v);
    coolness = Some(rcool);
    maybe_v
});
let coolness = coolness.unwrap(); // unwrap, ew

// Do some stuff with the coolness it doesn't really matter what
inspect_coolness(coolness);

This is because while F is bounded by FnOnce, it is under no obligation to call it, which in this example means there is no guarnatee that coolness is initialized. This proposed change introduces a new return value R which allows callers to transport this data out of F. Notably it also commits the replace_entry_with to calling F, since there is no other way to generate an R. With this, our example becomes:

/// Extract the coolness of an item, possibly destroying it
fn extract_coolness<V>(v: V) -> (Option<V>, Coolness) {
    /// ...
}

let entry: RawOccupiedEntryMut< _, _, _, _> = unimplemented!();

let (entry, coolness) = entry.replace_entry_with(|_k, v| extract_coolness(v));

// Do some stuff with the coolness it doesn't really matter what
inspect_coolness(coolness);

An alternative to this change is to introduce a new API with a new name instead of modifying the existing one as this is a breaking change.

If a user has a `RawOccupiedEntryMut`, they should be able to use `replace_entry_with` to conditionally remove it. However, if a user wishes to extract data from this entity during the removal process, the current api necessitates the use of `unwrap`:

```rust
/// Extract the coolness of an item, possibly destroying it
fn extract_coolness<V>(v: V) -> (Option<V>, Coolness) {
    /// ...
}

let entry: RawOccupiedEntryMut< _, _, _, _> = unimplemented!();

let mut coolness = None;
let entry = entry.replace_entry_with(|_k, v| {
    let (maybe_v, rcool) = extract_coolness(v);
    coolness = Some(rcool);
    maybe_v
});
let coolness = coolness.unwrap(); // unwrap, ew

// Do some stuff with the coolness it doesn't really matter what
inspect_coolness(coolness);
```

This is because while `F` is bounded by `FnOnce`, it is under no obligation to call it, which in this example means there is no guarnatee that `coolness` is initialized. This proposed change introduces a new return value `R` which allows callers to transport this data out of `F`. Notably it also commits the `replace_entry_with` to calling `F`, since there is no other way to generate an `R`. With this, our example becomes:

```rust
/// Extract the coolness of an item, possibly destroying it
fn extract_coolness<V>(v: V) -> (Option<V>, Coolness) {
    /// ...
}

let entry: RawOccupiedEntryMut< _, _, _, _> = unimplemented!();

let (entry, coolness) = entry.replace_entry_with(|_k, v| extract_coolness(v));

// Do some stuff with the coolness it doesn't really matter what
inspect_coolness(coolness);
```
@Daniel-Aaron-Bloom
Copy link
Author

Is there any interest in this change?

@bors
Copy link
Contributor

bors commented Apr 21, 2023

☔ The latest upstream changes (presumably #425) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu
Copy link
Member

Amanieu commented Apr 24, 2023

I spent some time thinking about this change. I don't think it is beneficial because it makes the common case where you don't want to return a value more complicated since you need to extract the new RawEntryMut from the returned tuple. Instead, it is better to let the uncommon case be more complex with the additional unwrap, which is going to be optimized away by the compiler anyways in release mode.

@Daniel-Aaron-Bloom
Copy link
Author

Daniel-Aaron-Bloom commented Apr 24, 2023

It is only a .0 to get the entry, but fair enough. I may just be overly biased against unwrap. Any consideration to having it as a separate API? Or is the burden too much for such little gains (unwrap haters unite!)?

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2023

Long-term, I am planning on deprecating the whole RawEntry API and replacing it with a HashTable type which is somewhere between RawTable and HashSet. It will have a 100% safe API but allow customized hashing/comparison like RawTable.

@Amanieu
Copy link
Member

Amanieu commented Aug 31, 2023

I would like to deprecate the raw entry API in favor of HashTable in #466. Could you have a look at that to see if it addresses your use case better?

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.

3 participants