-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add HtSP, HtSS, HtSU, SetS #4415
Conversation
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 it is to address #4261 ?
Overall I agree with the change but not sure if names of new functions are good ones. It's not clear what they are supposed to do just from the name, IMHO.
Also, since many of the calls use default arguments, what about creating a wrapper called something like ht_..._new_default_opts()
?
@ret2libc please take a look at this PR.
The PR looks good overall, I'll do a full review once it's ready for review. I agree however the names might not be ideal... 1) i think we should add some explanation for all existing ht (or at least this new ones) 2) instead of |
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.
Would do a full review when it is ready as well. Also agree with ret2libc and xvilka when it comes to the naming.
@ret2libc Do you mean to replace 'HtPP * ht_pp_new_s2p()' with 'HtPP * ht_sp_new()' or to add a whole bunch of HtSP API? The second one seems not less confusing because HtSP still be HtPP. Not to mention HtSS. |
Your checklist for this pull request
Detailed description
ht_*_new0()
are removedfreefn
field of HT options was renamed tofiniKV
finiKV_user
fields was added to HT optionsht_*_new_opt_size
APIHtSP
andHtUP
are created withValueFree
callback that allows to reduce extra LOC and prevent bugsSetP
replaced withSetS
(based onHtSP
)rz_th_ht_*_new0()
andrz_th_ht_*_new_opt()
are replaced withrz_th_ht_*_new(HtXX *)