Add wrapper for tiledb_vfs_ls_recursive.
#363
Conversation
teo-tsirpanis
force-pushed
the
ls-recursive
branch
from
8a2ea31 to
e2192dc
Compare
| @@ -464,7 +464,7 @@ func (v *VFS) NumOfFragmentsInPath(path string) (int, error) { | |||
| Vfs: v, | |||
| } | |||
| data := pointer.Save(&numOfFragmentsData) | |||
| defer C.free(data) | |||
| defer pointer.Unref(data) | |||
| @@ -635,6 +635,7 @@ func (v *VFS) List(path string) ([]string, []string, error) { | |||
| Vfs: v, | |||
| } | |||
| data := pointer.Save(&folderData) | |||
| defer pointer.Unref(data) | |||
vfs.go
Outdated
| @@ -643,3 +644,52 @@ func (v *VFS) List(path string) ([]string, []string, error) { | |||
|
|
|||
| return folderData.Folders, folderData.Files, nil | |||
| } | |||
|
|
|||
| type visitRecursiveCallback = func(path string, size uint64) (doContinue bool, err error) | |||
There was a problem hiding this comment.
We might want to export this type because VisitRecursive may otherwise be annoying to use: Not sure if all IDEs display non-exported types. If an IDE doesn't, you have to look at TileDB-Go's source code to figure out what function signature is expected here.
| type visitRecursiveCallback = func(path string, size uint64) (doContinue bool, err error) | |
| type VisitRecursiveCallback = func(path string, size uint64) (doContinue bool, err error) |
There was a problem hiding this comment.
I didn't want to copy-paste the callback's signature, but also didn't want to export a type without it being needed, but if some IDEs might not like it sure, done.
| return 0 | ||
| } | ||
|
|
||
| doContinue, err := state.callback(C.GoStringN(path, C.int(path_len)), uint64(size)) |
There was a problem hiding this comment.
If the callback panics we should catch and rethrow it after the call to ls_recursive like we do with the errors, right? I had done a similar thing in C# with exceptions.
There was a problem hiding this comment.
Do you mean using recover() to catch and rethrow an error if the callback panics? IMO if a user writes a callback that panics that's what should happen but @NullHypothesis please correct me if I am wrong.
For example I tried this in examples_lib/vfs.go and it panics, I think that's what would be expected.
err = vfs.VisitRecursive(dirA, func(path string, size uint64) (bool, error) {
panic("Test")
return false, nil
})There was a problem hiding this comment.
Yes it should still panic, but I'm not sure what is the behavior if we panic in a Go function that is called from C code, and whether we should have additional logic to handle it. I know that in C# this is not supported (because there is no standardized ABI for exceptions in non-Windows), but don't know anything about that in Go.
| callback: callback, | ||
| lastError: nil, | ||
| } | ||
| data := pointer.Save(state) |
There was a problem hiding this comment.
cgo.Handle provides the same functionality as this package inbox. Using it is tracked in SC-58131.
teo-tsirpanis
force-pushed
the
ls-recursive
branch
from
7e8744f to
a752ca9
Compare
| @@ -1,12 +1,13 @@ | |||
| #ifndef CLIBRARY_H | |||
| #define CLIBRARY_H | |||
|
|
|||
| #include <tiledb/tiledb.h> | |||
| #include <tiledb/tiledb_experimental.h> | |||
There was a problem hiding this comment.
We could create a vfs_experimental.go and clibrary_experimental.{h,c} to keep non experimental and experimental features separate
There was a problem hiding this comment.
Yeah, it looks like the experimental tag was removed and we added the _experimental.go suffix to TileDB-Go sources in #302
Initially my vote was to add the build flags that were missing at the top of these _experimental.go files, but after seeing the above PRs it looks like we actually don't want the experimental tag and it should not be present at the top of these files. So.. LGTM! 👍
SC-61002