Skip to content

Further prefix ops#59

Merged
declanvk merged 1 commit intodeclanvk:mainfrom
lucascool12:further_prefix_ops
Jul 20, 2025
Merged

Further prefix ops#59
declanvk merged 1 commit intodeclanvk:mainfrom
lucascool12:further_prefix_ops

Conversation

@lucascool12
Copy link
Contributor

After trying to use the prefix operations implemented in #50, I ran into some missing operations, which I implemented here.

Adds more operations that allow complex manipulation of trees using prefix operations.
Such as get_prefix_key_value_mut which returns the stored key together with a mutable reference to the value.
An entry api has been added for force_insert operations, this entry api has been added to the fuzzer.

@lucascool12 lucascool12 force-pushed the further_prefix_ops branch 2 times, most recently from c2cfb9d to 9c6c369 Compare July 13, 2025 14:24
}))
},
ForceInsertPoint::InsertPoint(insert_point) => match insert_point.insert_kind {
Exact { leaf_node_ptr } => {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Exact { leaf_node_ptr } => {
InsertKind::Exact { leaf_node_ptr } => {

And could you remove the use [...]::InsertKind::Exact from somewhere up above? I think I missed it last review

fn is_send<T: Send>() {}
fn is_sync<T: Sync>() {}

fn prefix_is_send<'a, K: Sync + 'a, V: Sync + 'a, A: Sync + Allocator + 'a>() {
Copy link
Owner

Choose a reason for hiding this comment

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

nit: these are named prefix_ when they're testing subtree iter

@declanvk
Copy link
Owner

declanvk commented Jul 15, 2025

I've also pondered the naming of these APIs and I'll like to rename them to:

fn force_insert -> fn prefix_insert
fn force_entry -> fn prefix_entry
fn get_prefix -> fn prefix_get
fn get_prefix_mut -> fn prefix_get_mut
fn get_prefix_key_value -> fn prefix_get_key_value
fn get_prefix_key_value_mut -> fn prefix_get_key_value_mut

My thinking was that then all the prefix operations are grouped by starting with the same thing. Also would have symmetry with the existing fn prefix iterator method.

Do you have any preferences around these names?

@declanvk
Copy link
Owner

Also would you mind updating the CHANGELOG for any new public APIs?

@lucascool12
Copy link
Contributor Author

I've also pondered the naming of these APIs and I'll like to rename them to:

fn force_insert -> fn prefix_insert
fn force_entry -> fn prefix_entry
fn get_prefix -> fn prefix_get
fn get_prefix_mut -> fn prefix_get_mut
fn get_prefix_key_value -> fn prefix_get_key_value
fn get_prefix_key_value_mut -> fn prefix_get_key_value_mut

My thinking was that then all the prefix operations are grouped by starting with the same thing. Also would have symmetry with the existing fn prefix iterator method.

Do you have any preferences around these names?

I have no preferences for these names.
I agree that this makes more sense, I'll also change all the Force* names and such to Prefix*.

@lucascool12 lucascool12 force-pushed the further_prefix_ops branch 2 times, most recently from fe30b7c to a019229 Compare July 19, 2025 07:01
@lucascool12
Copy link
Contributor Author

Alright, I think I addressed everything you mentioned.

Adds more operations that allow complex manipulation of trees using
prefix operations.
Such as `get_prefix_key_value_mut` which returns the stored key
together with a mutable reference to the value.
An entry api has been added for `force_insert` operations, this entry
api has been added to the fuzzer.
Copy link
Owner

@declanvk declanvk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing those comments!

@declanvk declanvk merged commit 556bf6c into declanvk:main Jul 20, 2025
4 checks passed
@declanvk
Copy link
Owner

I'll try to cut a release somewhat soon

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.

2 participants