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(map): Add GetNextKey and GetValueAndDeleteKey #411

Closed
wants to merge 3 commits into from

Conversation

chentao-kernel
Copy link
Contributor

Add GetNextKey() and GetValueAndDeleteKey() for map and map-low.

@chentao-kernel
Copy link
Contributor Author

Add GetNextKey() and GetValueAndDeleteKey() for map and map-low.

@geyslan Hi, geyslan Support map apis in todo list, please review when you are free, thanks lot.

map-low.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@geyslan
Copy link
Member

geyslan commented Apr 2, 2024

@chentao-kernel I'm baking the next version. Please tell me if you'll have time to finish this so it can be part of it. 👍🏼

@chentao-kernel
Copy link
Contributor Author

@chentao-kernel I'm baking the next version. Please tell me if you'll have time to finish this so it can be part of it. 👍🏼

Yes, i can finish it

@chentao-kernel
Copy link
Contributor Author

@chentao-kernel I'm baking the next version. Please tell me if you'll have time to finish this so it can be part of it. 👍🏼

Yes, i can finish it

@geyslan update it, please check again

Add GetNextKey(), GetValueAndDeleteKey() and GetValueAndDeleteKeyFlags()
to the map package.

Signed-off-by: Tao Chen <[email protected]>
Co-authored-by: Geyslan Gregório <[email protected]>
@geyslan
Copy link
Member

geyslan commented Apr 3, 2024

@chentao-kernel I raised this chentao-kernel#3.

Add GetNextKey(), GetValueAndDeleteKey() and GetValueAndDeleteKeyFlags()
to the map package.

Signed-off-by: Tao Chen <[email protected]>
Co-authored-by: Geyslan Gregório <[email protected]>
@chentao-kernel
Copy link
Contributor Author

@chentao-kernel I raised this chentao-kernel#3.

@geyslan hi, geyslan, thank you for your help, it was mergered.

@geyslan
Copy link
Member

geyslan commented Apr 5, 2024

@chentao-kernel, I was rethinking this API and realised that the initial TODO comment led to an erroneous GetNextKey signature as it is not performant (always allocating a buffer to store the fetched key value). This idea was brought up because in the current codebase we have some methods that work as helpers, already instantiating the values and doing the dirty work for the consumer. However, continuing with this approach is limiting the user to a sub-optimized API. So I bring the idea of from this new API we bring 1:1 wrappers and through them we create new helpers (with sugar and without performance guarantee).

I'll close this PR and soon send another one with this idea on top of your work.

In any case, it's a thing that should be addressed in the rest of the codebase, both for new and existing APIs.

@geyslan geyslan closed this Apr 5, 2024
@chentao-kernel
Copy link
Contributor Author

@chentao-kernel, I was rethinking this API and realised that the initial TODO comment led to an erroneous GetNextKey signature as it is not performant (always allocating a buffer to store the fetched key value). This idea was brought up because in the current codebase we have some methods that work as helpers, already instantiating the values and doing the dirty work for the consumer. However, continuing with this approach is limiting the user to a sub-optimized API. So I bring the idea of from this new API we bring 1:1 wrappers and through them we create new helpers (with sugar and without performance guarantee).

I'll close this PR and soon send another one with this idea on top of your work.

In any case, it's a thing that should be addressed in the rest of the codebase, both for new and existing APIs.

excellent,l saw your pr.

@geyslan geyslan mentioned this pull request Apr 9, 2024
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