-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement bulk delete #49
base: main
Are you sure you want to change the base?
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.
LGTM! Just some minor considerations
(Will approve once we have merged and released object_store_ffi
and updated the version here)
src/RustyObjectStore.jl
Outdated
c_paths = [Base.cconvert(Cstring, path) for path in paths] | ||
# Create an array of pointers to the Cstrings | ||
paths_array = [pointer(c_path) for c_path in c_paths] | ||
result = GC.@preserve paths c_paths paths_array config response event try |
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.
Quite a lot of GC.@preserve
, I think the c_paths
is redundant because Base.cconvert(Cstring, String)
is a noop IIRC, but it is better to be safe and leave it as you have here.
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 did some testing and Julia appears to be smart enough to convert Vector{String}
to Ptr{Ptr{Cchar}}
on the @ccall
directly. But I leave it for you to decide whether you trust it or not 😉
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.
changed it to pass Vector{String}
directly
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.
LGTM!
No description provided.