-
Notifications
You must be signed in to change notification settings - Fork 6
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
Migrate away from deprecated TileDB C APIs. #306
Conversation
if ret != C.TILEDB_OK { | ||
return "", "", TILEDB_INVALID, fmt.Errorf("Error getting member by index for group: %s", g.context.LastError()) | ||
} | ||
defer C.tiledb_string_free(&curi) |
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.
tiledb_string_free
returns a result, which I don't know what to do with 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.
Looks like it always returns TILEDB_OK
. I think it's safe to ignore the return value; especially since tiledb_group_get_member_by_index_v2
succeeded at this point, which is what the caller cares about.
CI is green. |
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, thanks for this!
@@ -274,42 +274,47 @@ func (g *Group) GetMemberCount() (uint64, error) { | |||
} | |||
|
|||
func (g *Group) GetMemberFromIndex(index uint64) (string, string, ObjectTypeEnum, error) { | |||
var curi *C.char | |||
defer C.free(unsafe.Pointer(curi)) |
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 wonder if this could have resulted in NULL pointer dereference segfaults if tiledb_group_get_member_by_index
returned with an error.
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 don't think so; free(null)
is a no-op.
if ret != C.TILEDB_OK { | ||
return "", "", TILEDB_INVALID, fmt.Errorf("Error getting member by index for group: %s", g.context.LastError()) | ||
} | ||
defer C.tiledb_string_free(&curi) |
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.
Looks like it always returns TILEDB_OK
. I think it's safe to ignore the return value; especially since tiledb_group_get_member_by_index_v2
succeeded at this point, which is what the caller cares about.
SC-45987
This PR updates TileDB-Go's usage of deprecated C APIs. The first commit migrates the
Group.GetMember***
functions to usetiledb_group_get_member_by_(index|name)_v2
, while also adding code to work with TileDB string handles. The second commit deprecatesQuery.SubmitAsync
, whose underlying C API has been deprecated since 2.15.After that, the only deprecated C APIs used by TileDB-Go are inside deprecated Go APIs.