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
Fix merge fn closure from pr385 #481
Fix merge fn closure from pr385 #481
Conversation
src/merge_operator.rs
Outdated
@@ -61,16 +62,25 @@ use std::mem; | |||
use std::ptr; | |||
use std::slice; | |||
|
|||
pub type MergeFn = fn(&[u8], Option<&[u8]>, &mut MergeOperands) -> Option<Vec<u8>>; | |||
//pub type MergeFn = fn(&[u8], Option<&[u8]>, &mut MergeOperands) -> Option<Vec<u8>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment really needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously not, it can be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I suspect things may get stuck here. I think this was a suggestion for you to update the PR @BoOTheFurious . But from your comment it seems like you expect someone else to do it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry… I've done it
…BoOTheFurious/rust-rocksdb into fix-merge-fn-closure_from_pr385
src/merge_operator.rs
Outdated
@@ -41,7 +41,8 @@ | |||
//!let mut opts = Options::default(); | |||
//! | |||
//!opts.create_if_missing(true); | |||
//!opts.set_merge_operator("test operator", concat_merge, None); | |||
//!opts.set_merge_operator_associative("test operator", concat_merge); | |||
//!//opts.set_merge_operator("test operator", concat_merge, partial_concat_merge); // if your merge is not associative |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this comment could be deleted as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops…
done !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Hello,
I 've got a use case which need to use stateful closure as merge function. As an old PR refers to my needs (PR#385 1 year old) I have rebase on current master. I have also integrated PR#480 to pass the test.
All works fine. I hope It will be accepted quickly.
Thanks @mrobakowski for the works on the initial PR.
Regards