From 4b6d7994efc0719865130f7292f7902e8d4eb6ca Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Apr 2023 15:12:33 +0300 Subject: [PATCH 01/27] Add `tiledb_group_get_member_by_***_v2`. --- tiledb/api/c_api/group/group_api.cc | 63 ++++++++++++++ .../group/group_api_external_experimental.h | 85 +++++++++++++++++-- 2 files changed, 142 insertions(+), 6 deletions(-) diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index c5d8feaf62c..9eeb2b84d06 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -41,6 +41,7 @@ #include "../buffer/buffer_api_internal.h" #include "../config/config_api_internal.h" #include "../context/context_api_internal.h" +#include "../string/string_api_internal.h" #include "group_api_internal.h" @@ -346,6 +347,27 @@ capi_return_t tiledb_group_get_member_by_index( return TILEDB_OK; } +capi_return_t tiledb_group_get_member_by_index_v2( + tiledb_group_handle_t* group, + uint64_t index, + tiledb_string_t** uri, + tiledb_object_t* type, + tiledb_string_t** name) { + ensure_group_is_valid(group); + ensure_output_pointer_is_valid(uri); + ensure_output_pointer_is_valid(type); + ensure_output_pointer_is_valid(name); + + auto&& [uri_str, object_type, name_str] = + group->group().member_by_index(index); + + *uri = tiledb_string_handle_t::make_handle(uri_str); + *type = static_cast(object_type); + *name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr; + + return TILEDB_OK; +} + capi_return_t tiledb_group_get_member_by_name( tiledb_group_handle_t* group, const char* name, @@ -369,6 +391,26 @@ capi_return_t tiledb_group_get_member_by_name( return TILEDB_OK; } +capi_return_t tiledb_group_get_member_by_name_v2( + tiledb_group_handle_t* group, + const char* name, + tiledb_string_t** uri, + tiledb_object_t* type) { + ensure_group_is_valid(group); + ensure_output_pointer_is_valid(uri); + ensure_output_pointer_is_valid(type); + + std::string uri_str; + sm::ObjectType object_type; + std::tie(uri_str, object_type, std::ignore, std::ignore) = + group->group().member_by_name(name); + + *uri = tiledb_string_handle_t::make_handle(std::move(uri_str)); + *type = static_cast(object_type); + + return TILEDB_OK; +} + capi_return_t tiledb_group_get_is_relative_uri_by_name( tiledb_group_handle_t* group, const char* name, uint8_t* is_relative) { ensure_group_is_valid(group); @@ -700,6 +742,17 @@ capi_return_t tiledb_group_get_member_by_index( ctx, group, index, uri, type, name); } +capi_return_t tiledb_group_get_member_by_index_v2( + tiledb_ctx_t* ctx, + tiledb_group_t* group, + uint64_t index, + tiledb_string_t** uri, + tiledb_object_t* type, + tiledb_string_t** name) TILEDB_NOEXCEPT { + return api_entry_context( + ctx, group, index, uri, type, name); +} + capi_return_t tiledb_group_get_member_by_name( tiledb_ctx_t* ctx, tiledb_group_t* group, @@ -710,6 +763,16 @@ capi_return_t tiledb_group_get_member_by_name( ctx, group, name, uri, type); } +capi_return_t tiledb_group_get_member_by_name_v2( + tiledb_ctx_t* ctx, + tiledb_group_t* group, + const char* name, + tiledb_string_t** uri, + tiledb_object_t* type) TILEDB_NOEXCEPT { + return api_entry_context( + ctx, group, name, uri, type); +} + capi_return_t tiledb_group_get_is_relative_uri_by_name( tiledb_ctx_t* ctx, tiledb_group_t* group, diff --git a/tiledb/api/c_api/group/group_api_external_experimental.h b/tiledb/api/c_api/group/group_api_external_experimental.h index d75effd73bc..3d3cb7ca617 100644 --- a/tiledb/api/c_api/group/group_api_external_experimental.h +++ b/tiledb/api/c_api/group/group_api_external_experimental.h @@ -423,11 +423,9 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_count( * @param ctx The TileDB context. * @param group An group opened in READ mode. * @param index index of member to fetch - * @param uri URI of member, The caller takes ownership - * of the c-string. + * @param uri URI of member. * @param type type of member - * @param name name of member, The caller takes ownership - * of the c-string. NULL if name was not set + * @param name name of member. NULL if name was not set * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index( @@ -447,6 +445,45 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index( * tiledb_group_t* group; * tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group); * tiledb_group_open(ctx, group, TILEDB_WRITE); + * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array"); + * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2"); + * + * tiledb_group_close(ctx, group); + * tiledb_group_open(ctx, group, TILEDB_READ); + * tiledb_string_t *uri, *name; + * tiledb_object_t type; + * tiledb_group_get_member_by_index_v2(ctx, group, 0, &uri, &type, &name); + * + * tiledb_string_free(uri); + * tiledb_string_free(name); + * + * @endcode + * + * @param ctx The TileDB context. + * @param group An group opened in READ mode. + * @param index index of member to fetch + * @param uri Handle to the URI of the member. + * @param type type of member + * @param name Handle to the name of the member. NULL if name was not set + * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. + */ +TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index_v2( + tiledb_ctx_t* ctx, + tiledb_group_t* group, + uint64_t index, + tiledb_string_t** uri, + tiledb_object_t* type, + tiledb_string_t** name) TILEDB_NOEXCEPT; + +/** + * Get a member of a group by name and details of group + * + * **Example:** + * + * @code{.c} + * tiledb_group_t* group; + * tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group); + * tiledb_group_open(ctx, group, TILEDB_WRITE); * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array", "array1"); * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2", * "group2"); @@ -464,8 +501,7 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index( * @param ctx The TileDB context. * @param group An group opened in READ mode. * @param name name of member to fetch - * @param uri URI of member, The caller takes ownership - * of the c-string. + * @param uri URI of member * @param type type of member * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ @@ -476,6 +512,43 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_name( char** uri, tiledb_object_t* type) TILEDB_NOEXCEPT; +/** + * Get a member of a group by name and details of group. + * + * **Example:** + * + * @code{.c} + * tiledb_group_t* group; + * tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group); + * tiledb_group_open(ctx, group, TILEDB_WRITE); + * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array", "array1"); + * tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2", + * "group2"); + * + * tiledb_group_close(ctx, group); + * tiledb_group_open(ctx, group, TILEDB_READ); + * tilledb_string_t *uri; + * tiledb_object_t type; + * tiledb_group_get_member_by_name(ctx, group, "array1", &uri, &type); + * + * tiledb_string_free(uri); + * + * @endcode + * + * @param ctx The TileDB context. + * @param group An group opened in READ mode. + * @param name name of member to fetch + * @param uri Handle to the URI of the member. + * @param type type of member + * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. + */ +TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_name_v2( + tiledb_ctx_t* ctx, + tiledb_group_t* group, + const char* name, + tiledb_string_t** uri, + tiledb_object_t* type) TILEDB_NOEXCEPT; + /* (clang format was butchering the tiledb_group_add_member() calls) */ /* clang-format off */ /** From 6fc5d1732a660be26d7136b46305d82f20b4118d Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Apr 2023 14:48:37 +0300 Subject: [PATCH 02/27] Deprecate the existing group member APIs. --- doc/policy/api_changes.md | 5 +++++ tiledb/api/c_api/group/group_api.cc | 8 ++++++++ .../api/c_api/group/group_api_external_experimental.h | 10 ++++++---- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/doc/policy/api_changes.md b/doc/policy/api_changes.md index ec935b877de..7e36268a78c 100644 --- a/doc/policy/api_changes.md +++ b/doc/policy/api_changes.md @@ -74,6 +74,11 @@ Function removal shall be announced one release cycle before removal, following - `tiledb_query_submit_async` +### 2.15.0..2.16.0 + +- `tiledb_group_get_member_by_index` +- `tiledb_group_get_member_by_name` + ## Deprecation History Generation
diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index 9eeb2b84d06..104be0e8b27 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -310,6 +310,10 @@ capi_return_t tiledb_group_get_member_by_index( ensure_output_pointer_is_valid(type); ensure_output_pointer_is_valid(name); + LOG_WARN( + "tiledb_group_get_member_by_index is deprecated. Please use " + "tiledb_group_get_member_by_index_v2 instead."); + char* tmp_uri = nullptr; char* tmp_name = nullptr; @@ -378,6 +382,10 @@ capi_return_t tiledb_group_get_member_by_name( ensure_output_pointer_is_valid(uri); ensure_output_pointer_is_valid(type); + LOG_WARN( + "tiledb_group_get_member_by_name is deprecated. Please use " + "tiledb_group_get_member_by_name_v2 instead."); + auto&& [uri_str, object_type, name_str, ignored_relative] = group->group().member_by_name(name); diff --git a/tiledb/api/c_api/group/group_api_external_experimental.h b/tiledb/api/c_api/group/group_api_external_experimental.h index 3d3cb7ca617..c69e0b134db 100644 --- a/tiledb/api/c_api/group/group_api_external_experimental.h +++ b/tiledb/api/c_api/group/group_api_external_experimental.h @@ -399,7 +399,8 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_count( tiledb_ctx_t* ctx, tiledb_group_t* group, uint64_t* count) TILEDB_NOEXCEPT; /** - * Get a member of a group by index and details of group + * Get a member of a group by index and details of group. + * Deprecated, use \p tiledb_group_get_member_by_index_v2 instead. * * **Example:** * @@ -428,7 +429,7 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_count( * @param name name of member. NULL if name was not set * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ -TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index( +TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_get_member_by_index( tiledb_ctx_t* ctx, tiledb_group_t* group, uint64_t index, @@ -476,7 +477,8 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index_v2( tiledb_string_t** name) TILEDB_NOEXCEPT; /** - * Get a member of a group by name and details of group + * Get a member of a group by name and details of group. + * Deprecated, use \p tiledb_group_get_member_by_name_v2. * * **Example:** * @@ -505,7 +507,7 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index_v2( * @param type type of member * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ -TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_name( +TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_get_member_by_name( tiledb_ctx_t* ctx, tiledb_group_t* group, const char* name, From c0f62f652cac6a8018dc4f9b8a473bd051f1061e Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Apr 2023 15:22:27 +0300 Subject: [PATCH 03/27] Use the new group member APIs. And add a common function to convert string handles to C++ strings. --- test/src/unit-capi-group.cc | 22 +++++++++++++++------- tiledb/sm/cpp_api/fragment_info.h | 5 +---- tiledb/sm/cpp_api/group_experimental.h | 24 +++++++++++------------- tiledb/sm/cpp_api/utils.h | 12 ++++++++++++ 4 files changed, 39 insertions(+), 24 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index 9cc363e703d..b1316e91c41 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -233,17 +233,25 @@ std::vector> GroupFx::read_group( tiledb_group_t* group) const { std::vector> ret; uint64_t count = 0; - char* uri; - tiledb_object_t type; - char* name; int rc = tiledb_group_get_member_count(ctx_, group, &count); REQUIRE(rc == TILEDB_OK); for (uint64_t i = 0; i < count; i++) { - rc = tiledb_group_get_member_by_index(ctx_, group, i, &uri, &type, &name); + tiledb_string_t* uri; + tiledb_object_t type; + tiledb_string_t* name; + rc = + tiledb_group_get_member_by_index_v2(ctx_, group, i, &uri, &type, &name); REQUIRE(rc == TILEDB_OK); - ret.emplace_back(uri, type); - std::free(uri); - std::free(name); + const char* uri_str; + size_t uri_size; + rc = tiledb_string_view(uri, &uri_str, &uri_size); + ret.emplace_back(std::string(uri_str, uri_size), type); + rc = tiledb_string_free(&uri); + REQUIRE(rc == TILEDB_OK); + if (name) { + rc = tiledb_string_free(&name); + REQUIRE(rc == TILEDB_OK); + } } return ret; } diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index 72f9cf91ec4..c79af08fcda 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -99,10 +99,7 @@ class FragmentInfo { ctx.ptr().get(), fragment_info_.get(), fid, &name)); auto name_ptr = std::unique_ptr(name); - const char* name_c; - size_t length; - ctx.handle_error(tiledb_string_view(name_ptr.get(), &name_c, &length)); - return std::string(name_c, length); + return impl::handle_to_string(name_ptr); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index d23db51ae84..ce0e03d8ced 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -413,31 +413,29 @@ class Group { tiledb::Object member(uint64_t index) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - char* uri; + tiledb_string_t* uri; tiledb_object_t type; - char* name; - ctx.handle_error(tiledb_group_get_member_by_index( + tiledb_string_t* name; + ctx.handle_error(tiledb_group_get_member_by_index_v2( c_ctx, group_.get(), index, &uri, &type, &name)); - std::string uri_str(uri); - std::free(uri); + auto uri_ptr = std::unique_ptr(uri); + auto name_ptr = std::unique_ptr(name); std::optional name_opt = std::nullopt; if (name != nullptr) { - name_opt = name; - std::free(name); + name_opt = impl::handle_to_string(name_ptr); } - return tiledb::Object(type, uri_str, name_opt); + return tiledb::Object(type, impl::handle_to_string(uri_ptr), name_opt); } tiledb::Object member(std::string name) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - char* uri; + tiledb_string_t* uri; tiledb_object_t type; - ctx.handle_error(tiledb_group_get_member_by_name( + ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); - std::string uri_str(uri); - std::free(uri); - return tiledb::Object(type, uri_str, name); + auto uri_ptr = std::unique_ptr(uri); + return tiledb::Object(type, impl::handle_to_string(uri_ptr), name); } /** diff --git a/tiledb/sm/cpp_api/utils.h b/tiledb/sm/cpp_api/utils.h index 71763fbe196..0a50a04d0b6 100644 --- a/tiledb/sm/cpp_api/utils.h +++ b/tiledb/sm/cpp_api/utils.h @@ -395,6 +395,18 @@ inline void check_config_error(tiledb_error_t* err) { } } +template +inline std::string handle_to_string( + const std::unique_ptr& str) { + const char* name_c; + size_t length; + if (tiledb_status(tiledb_string_view(str.get(), &name_c, &length)) != + TILEDB_OK) { + throw std::runtime_error("tiledb_string_view failed."); + } + return std::string(name_c, length); +} + } // namespace impl } // namespace tiledb From 8dde4aaeb500f817f1cdb2632e8a76f939831a38 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Apr 2023 18:11:25 +0300 Subject: [PATCH 04/27] Add a dedicated class to manage lifetime of string handles. It is much easier to use and correctly handles null handles. --- tiledb/CMakeLists.txt | 1 + tiledb/sm/cpp_api/deleter.h | 7 -- tiledb/sm/cpp_api/fragment_info.h | 9 +- tiledb/sm/cpp_api/group_experimental.h | 22 ++--- tiledb/sm/cpp_api/string_handle_holder.h | 114 +++++++++++++++++++++++ tiledb/sm/cpp_api/utils.h | 12 --- 6 files changed, 127 insertions(+), 38 deletions(-) create mode 100644 tiledb/sm/cpp_api/string_handle_holder.h diff --git a/tiledb/CMakeLists.txt b/tiledb/CMakeLists.txt index cc59e5e3876..6ec46dd88c6 100644 --- a/tiledb/CMakeLists.txt +++ b/tiledb/CMakeLists.txt @@ -110,6 +110,7 @@ if (TILEDB_CPP_API) ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/query_experimental.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/schema_base.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/stats.h + ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/string_handle_holder.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray_experimental.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/type.h diff --git a/tiledb/sm/cpp_api/deleter.h b/tiledb/sm/cpp_api/deleter.h index c72d8ef71a1..db2c8be5a01 100644 --- a/tiledb/sm/cpp_api/deleter.h +++ b/tiledb/sm/cpp_api/deleter.h @@ -139,13 +139,6 @@ class Deleter { tiledb_consolidation_plan_free(&p); } - void operator()(tiledb_string_t* p) const { - capi_status_t result = tiledb_status(tiledb_string_free(&p)); - if (result != TILEDB_OK) { - log_warn("Could not free string; Error code: " + std::to_string(result)); - } - } - private: /* ********************************* */ /* PRIVATE ATTRIBUTES */ diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index c79af08fcda..57309b14179 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -38,6 +38,7 @@ #include "deleter.h" #include "exception.h" #include "object.h" +#include "string_handle_holder.h" #include "tiledb.h" #include "type.h" @@ -94,12 +95,10 @@ class FragmentInfo { /** Returns the name of the fragment with the given index. */ std::string fragment_name(uint32_t fid) const { auto& ctx = ctx_.get(); - tiledb_string_t* name; + impl::StringHandleHolder name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( - ctx.ptr().get(), fragment_info_.get(), fid, &name)); - auto name_ptr = - std::unique_ptr(name); - return impl::handle_to_string(name_ptr); + ctx.ptr().get(), fragment_info_.get(), fid, name.c_ptr())); + return name.str(); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index ce0e03d8ced..a5582255ed7 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -36,6 +36,7 @@ #define TILEDB_CPP_API_GROUP_EXPERIMENTAL_H #include "context.h" +#include "string_handle_holder.h" #include "tiledb.h" namespace tiledb { @@ -413,29 +414,22 @@ class Group { tiledb::Object member(uint64_t index) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - tiledb_string_t* uri; + impl::StringHandleHolder uri; tiledb_object_t type; - tiledb_string_t* name; + impl::StringHandleHolder name; ctx.handle_error(tiledb_group_get_member_by_index_v2( - c_ctx, group_.get(), index, &uri, &type, &name)); - auto uri_ptr = std::unique_ptr(uri); - auto name_ptr = std::unique_ptr(name); - std::optional name_opt = std::nullopt; - if (name != nullptr) { - name_opt = impl::handle_to_string(name_ptr); - } - return tiledb::Object(type, impl::handle_to_string(uri_ptr), name_opt); + c_ctx, group_.get(), index, uri.c_ptr(), &type, name.c_ptr())); + return tiledb::Object(type, uri.str(), name.str_opt()); } tiledb::Object member(std::string name) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - tiledb_string_t* uri; + impl::StringHandleHolder uri; tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( - c_ctx, group_.get(), name.c_str(), &uri, &type)); - auto uri_ptr = std::unique_ptr(uri); - return tiledb::Object(type, impl::handle_to_string(uri_ptr), name); + c_ctx, group_.get(), name.c_str(), uri.c_ptr(), &type)); + return tiledb::Object(type, uri.str(), name); } /** diff --git a/tiledb/sm/cpp_api/string_handle_holder.h b/tiledb/sm/cpp_api/string_handle_holder.h new file mode 100644 index 00000000000..53713298847 --- /dev/null +++ b/tiledb/sm/cpp_api/string_handle_holder.h @@ -0,0 +1,114 @@ +/** + * @file string_handle_holder.h + * + * @section LICENSE + * + * The MIT License + * + * @copyright Copyright (c) 2017-2023 TileDB, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * @section DESCRIPTION + * + * This file declares the C++ API for the TileDB StringHandleHolder object. + */ + +#ifndef TILEDB_CPP_API_STRING_HANDLE_HOLDER_H +#define TILEDB_CPP_API_STRING_HANDLE_HOLDER_H + +#include +#include +#include +#include "tiledb.h" + +namespace tiledb::impl { + +/** + * Manages the lifetime of a tiledb_string_t* handle and provides operations on + * it. + */ +class StringHandleHolder { + public: + StringHandleHolder() { + string_ = nullptr; + } + ~StringHandleHolder() { + if (string_ == nullptr) { + return; + } + + capi_status_t result = tiledb_status(tiledb_string_free(&string_)); + if (result != TILEDB_OK) { + log_warn("Could not free string; Error code: " + std::to_string(result)); + } + } + + // Disable copy and move. + StringHandleHolder(const StringHandleHolder&) = delete; + StringHandleHolder operator=(const StringHandleHolder&) = delete; + StringHandleHolder(const StringHandleHolder&&) = delete; + StringHandleHolder operator=(const StringHandleHolder&&) = delete; + + /** + * Returns a tiledb_string_t** pointer to be passed in native code. + * This method should be called only once. + */ + inline tiledb_string_t** c_ptr() { + // We should call this function only once when the object is uninitialized. + assert(string_ == nullptr); + return &string_; + } + + /** + * Returns a C++ string with the handle's data. + * + * If the handle is null, returns a defaullt value, which is the empty string + * if not specified. + */ + inline std::string str(const std::string& default_value = "") const { + return str_opt().value_or(default_value); + } + + /** + * Returns a C++ string with the handle's data, or nullopt if the handle is + * null. + */ + std::optional str_opt() const { + if (string_ == nullptr) { + return std::nullopt; + } + const char* c; + size_t size; + capi_status_t status = + tiledb_status(tiledb_string_view(string_, &c, &size)); + if (status != TILEDB_OK) { + throw std::runtime_error( + "Could not view string; Error code: " + std::to_string(status)); + } + return std::string(c, size); + } + + private: + tiledb_string_t* string_; +}; + +} // namespace tiledb::impl + +#endif // TILEDB_CPP_API_STRING_HANDLE_HOLDER_H diff --git a/tiledb/sm/cpp_api/utils.h b/tiledb/sm/cpp_api/utils.h index 0a50a04d0b6..71763fbe196 100644 --- a/tiledb/sm/cpp_api/utils.h +++ b/tiledb/sm/cpp_api/utils.h @@ -395,18 +395,6 @@ inline void check_config_error(tiledb_error_t* err) { } } -template -inline std::string handle_to_string( - const std::unique_ptr& str) { - const char* name_c; - size_t length; - if (tiledb_status(tiledb_string_view(str.get(), &name_c, &length)) != - TILEDB_OK) { - throw std::runtime_error("tiledb_string_view failed."); - } - return std::string(name_c, length); -} - } // namespace impl } // namespace tiledb From 0ce0af46f881aa2ac735c9302aebf823458e8384 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:02:06 +0300 Subject: [PATCH 05/27] Make `StringHandleHolder` RAII-compliant and rename it to `CAPIString`. --- tiledb/CMakeLists.txt | 2 +- .../{string_handle_holder.h => capi_string.h} | 39 +++++++------------ tiledb/sm/cpp_api/fragment_info.h | 8 ++-- tiledb/sm/cpp_api/group_experimental.h | 19 +++++---- 4 files changed, 31 insertions(+), 37 deletions(-) rename tiledb/sm/cpp_api/{string_handle_holder.h => capi_string.h} (71%) diff --git a/tiledb/CMakeLists.txt b/tiledb/CMakeLists.txt index 6ec46dd88c6..868e20282af 100644 --- a/tiledb/CMakeLists.txt +++ b/tiledb/CMakeLists.txt @@ -110,7 +110,7 @@ if (TILEDB_CPP_API) ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/query_experimental.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/schema_base.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/stats.h - ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/string_handle_holder.h + ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/capi_string.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray_experimental.h ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/type.h diff --git a/tiledb/sm/cpp_api/string_handle_holder.h b/tiledb/sm/cpp_api/capi_string.h similarity index 71% rename from tiledb/sm/cpp_api/string_handle_holder.h rename to tiledb/sm/cpp_api/capi_string.h index 53713298847..4e8be6dffb0 100644 --- a/tiledb/sm/cpp_api/string_handle_holder.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -1,5 +1,5 @@ /** - * @file string_handle_holder.h + * @file capi_string.h * * @section LICENSE * @@ -27,11 +27,11 @@ * * @section DESCRIPTION * - * This file declares the C++ API for the TileDB StringHandleHolder object. + * This file declares the C++ API for the TileDB CAPIString object. */ -#ifndef TILEDB_CPP_API_STRING_HANDLE_HOLDER_H -#define TILEDB_CPP_API_STRING_HANDLE_HOLDER_H +#ifndef TILEDB_CPP_API_CAPI_STRING_H +#define TILEDB_CPP_API_CAPI_STRING_H #include #include @@ -44,12 +44,13 @@ namespace tiledb::impl { * Manages the lifetime of a tiledb_string_t* handle and provides operations on * it. */ -class StringHandleHolder { +class CAPIString { public: - StringHandleHolder() { - string_ = nullptr; + CAPIString(tiledb_string_t*&& handle) { + string_ = handle; + handle = nullptr; } - ~StringHandleHolder() { + ~CAPIString() { if (string_ == nullptr) { return; } @@ -61,20 +62,10 @@ class StringHandleHolder { } // Disable copy and move. - StringHandleHolder(const StringHandleHolder&) = delete; - StringHandleHolder operator=(const StringHandleHolder&) = delete; - StringHandleHolder(const StringHandleHolder&&) = delete; - StringHandleHolder operator=(const StringHandleHolder&&) = delete; - - /** - * Returns a tiledb_string_t** pointer to be passed in native code. - * This method should be called only once. - */ - inline tiledb_string_t** c_ptr() { - // We should call this function only once when the object is uninitialized. - assert(string_ == nullptr); - return &string_; - } + CAPIString(const CAPIString&) = delete; + CAPIString operator=(const CAPIString&) = delete; + CAPIString(const CAPIString&&) = delete; + CAPIString operator=(const CAPIString&&) = delete; /** * Returns a C++ string with the handle's data. @@ -82,7 +73,7 @@ class StringHandleHolder { * If the handle is null, returns a defaullt value, which is the empty string * if not specified. */ - inline std::string str(const std::string& default_value = "") const { + std::string str(const std::string& default_value = "") const { return str_opt().value_or(default_value); } @@ -111,4 +102,4 @@ class StringHandleHolder { } // namespace tiledb::impl -#endif // TILEDB_CPP_API_STRING_HANDLE_HOLDER_H +#endif // TILEDB_CPP_API_CAPI_STRING_H diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index 57309b14179..cb234db4b4e 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -34,11 +34,11 @@ #define TILEDB_CPP_API_FRAGMENT_INFO_H #include "array_schema.h" +#include "capi_string.h" #include "context.h" #include "deleter.h" #include "exception.h" #include "object.h" -#include "string_handle_holder.h" #include "tiledb.h" #include "type.h" @@ -95,10 +95,10 @@ class FragmentInfo { /** Returns the name of the fragment with the given index. */ std::string fragment_name(uint32_t fid) const { auto& ctx = ctx_.get(); - impl::StringHandleHolder name; + tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( - ctx.ptr().get(), fragment_info_.get(), fid, name.c_ptr())); - return name.str(); + ctx.ptr().get(), fragment_info_.get(), fid, &name)); + return impl::CAPIString(std::move(name)).str(); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index a5582255ed7..afa16e6688c 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -35,8 +35,8 @@ #ifndef TILEDB_CPP_API_GROUP_EXPERIMENTAL_H #define TILEDB_CPP_API_GROUP_EXPERIMENTAL_H +#include "capi_string.h" #include "context.h" -#include "string_handle_holder.h" #include "tiledb.h" namespace tiledb { @@ -414,22 +414,25 @@ class Group { tiledb::Object member(uint64_t index) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - impl::StringHandleHolder uri; + tiledb_string_t* uri; tiledb_object_t type; - impl::StringHandleHolder name; + tiledb_string_t* name; ctx.handle_error(tiledb_group_get_member_by_index_v2( - c_ctx, group_.get(), index, uri.c_ptr(), &type, name.c_ptr())); - return tiledb::Object(type, uri.str(), name.str_opt()); + c_ctx, group_.get(), index, &uri, &type, &name)); + return tiledb::Object( + type, + impl::CAPIString(std::move(uri)).str(), + impl::CAPIString(std::move(name)).str_opt()); } tiledb::Object member(std::string name) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - impl::StringHandleHolder uri; + tiledb_string_t* uri; tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( - c_ctx, group_.get(), name.c_str(), uri.c_ptr(), &type)); - return tiledb::Object(type, uri.str(), name); + c_ctx, group_.get(), name.c_str(), &uri, &type)); + return tiledb::Object(type, impl::CAPIString(std::move(uri)).str(), name); } /** From 6959e7ba5bbe1bca082b684078a5c05792392d6a Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:08:56 +0300 Subject: [PATCH 06/27] Fix a potential memory leak. --- tiledb/api/c_api/group/group_api.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index 104be0e8b27..d87651a0f62 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -367,7 +367,12 @@ capi_return_t tiledb_group_get_member_by_index_v2( *uri = tiledb_string_handle_t::make_handle(uri_str); *type = static_cast(object_type); - *name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr; + try { + *name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr; + } catch (...) { + tiledb_string_handle_t::break_handle(*uri); + throw; + } return TILEDB_OK; } From 24847c17a81f0de251373a4246514e62bca285e8 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:10:54 +0300 Subject: [PATCH 07/27] Address more PR feedback. --- test/src/unit-capi-group.cc | 3 ++- tiledb/api/c_api/group/group_api.cc | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index b1316e91c41..82d3eb98f5d 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -245,9 +245,10 @@ std::vector> GroupFx::read_group( const char* uri_str; size_t uri_size; rc = tiledb_string_view(uri, &uri_str, &uri_size); + CHECK(rc == TILEDB_OK); ret.emplace_back(std::string(uri_str, uri_size), type); rc = tiledb_string_free(&uri); - REQUIRE(rc == TILEDB_OK); + CHECK(rc == TILEDB_OK); if (name) { rc = tiledb_string_free(&name); REQUIRE(rc == TILEDB_OK); diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index d87651a0f62..edb0b97f8d4 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -368,7 +368,9 @@ capi_return_t tiledb_group_get_member_by_index_v2( *uri = tiledb_string_handle_t::make_handle(uri_str); *type = static_cast(object_type); try { - *name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr; + *name = name_str.has_value() ? + tiledb_string_handle_t::make_handle(*name_str) : + nullptr; } catch (...) { tiledb_string_handle_t::break_handle(*uri); throw; From cc325c9d8173a201ce908d647dd9f54875d46cad Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:28:59 +0300 Subject: [PATCH 08/27] Mark `CAPIString`'s constructor with `noexcept`. --- tiledb/sm/cpp_api/capi_string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 4e8be6dffb0..88ec3d66f7c 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -46,7 +46,7 @@ namespace tiledb::impl { */ class CAPIString { public: - CAPIString(tiledb_string_t*&& handle) { + CAPIString(tiledb_string_t*&& handle) noexcept { string_ = handle; handle = nullptr; } From 68b7063ff7d2e8c30ef99c8dc284dccf3606510f Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:38:41 +0300 Subject: [PATCH 09/27] Use static methods to expose `CAPIString`'s functionality, explicitly accounting for optional strings. --- tiledb/sm/cpp_api/capi_string.h | 23 +++++++++++++---------- tiledb/sm/cpp_api/fragment_info.h | 2 +- tiledb/sm/cpp_api/group_experimental.h | 7 ++++--- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 88ec3d66f7c..0af285bf5a6 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -70,21 +70,24 @@ class CAPIString { /** * Returns a C++ string with the handle's data. * - * If the handle is null, returns a defaullt value, which is the empty string - * if not specified. + * If the handle is null returns nullopt. */ - std::string str(const std::string& default_value = "") const { - return str_opt().value_or(default_value); + static std::optional to_string_optional( + tiledb_string_t*&& handle) { + if (handle == nullptr) { + return std::nullopt; + } + return to_string(std::move(handle)); } /** - * Returns a C++ string with the handle's data, or nullopt if the handle is - * null. + * Returns a C++ string with the handle's data. */ - std::optional str_opt() const { - if (string_ == nullptr) { - return std::nullopt; - } + static std::string to_string(tiledb_string_t*&& handle) { + return CAPIString(std::move(handle)).str(); + } + + std::string str() const { const char* c; size_t size; capi_status_t status = diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index cb234db4b4e..9ae2735e02b 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -98,7 +98,7 @@ class FragmentInfo { tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( ctx.ptr().get(), fragment_info_.get(), fid, &name)); - return impl::CAPIString(std::move(name)).str(); + return impl::CAPIString::to_string(std::move(name)); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index afa16e6688c..ca77485a028 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -421,8 +421,8 @@ class Group { c_ctx, group_.get(), index, &uri, &type, &name)); return tiledb::Object( type, - impl::CAPIString(std::move(uri)).str(), - impl::CAPIString(std::move(name)).str_opt()); + impl::CAPIString::to_string(std::move(uri)), + impl::CAPIString::to_string_optional(std::move(name))); } tiledb::Object member(std::string name) const { @@ -432,7 +432,8 @@ class Group { tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); - return tiledb::Object(type, impl::CAPIString(std::move(uri)).str(), name); + return tiledb::Object( + type, impl::CAPIString::to_string(std::move(uri)), name); } /** From 8c2fd53b316c17d28d82d1c6824505badae198f2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 10 Apr 2023 15:52:11 +0300 Subject: [PATCH 10/27] Keep testing the old function. --- test/src/unit-capi-group.cc | 40 ++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index 82d3eb98f5d..4f42b33fdc6 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -236,23 +236,35 @@ std::vector> GroupFx::read_group( int rc = tiledb_group_get_member_count(ctx_, group, &count); REQUIRE(rc == TILEDB_OK); for (uint64_t i = 0; i < count; i++) { - tiledb_string_t* uri; + std::string uri; tiledb_object_t type; - tiledb_string_t* name; - rc = - tiledb_group_get_member_by_index_v2(ctx_, group, i, &uri, &type, &name); - REQUIRE(rc == TILEDB_OK); - const char* uri_str; - size_t uri_size; - rc = tiledb_string_view(uri, &uri_str, &uri_size); - CHECK(rc == TILEDB_OK); - ret.emplace_back(std::string(uri_str, uri_size), type); - rc = tiledb_string_free(&uri); - CHECK(rc == TILEDB_OK); - if (name) { - rc = tiledb_string_free(&name); + // TODO: When tiledb_group_get_member_by_index gets removed, replace the + // following value generation and if with the statement-true of the if. + bool use_v2 = GENERATE(true, false); + if (use_v2) { + tiledb_string_t *uri_handle, *name_handle; + rc = tiledb_group_get_member_by_index_v2( + ctx_, group, i, &uri_handle, &type, &name_handle); + REQUIRE(rc == TILEDB_OK); + const char* uri_str; + size_t uri_size; + rc = tiledb_string_view(uri_handle, &uri_str, &uri_size); + CHECK(rc == TILEDB_OK); + uri = std::string(uri_str, uri_size); + rc = tiledb_string_free(&uri_handle); + CHECK(rc == TILEDB_OK); + if (name_handle) { + rc = tiledb_string_free(&name_handle); + CHECK(rc == TILEDB_OK); + } + } else { + char *uri_ptr, *name_ptr; + rc = tiledb_group_get_member_by_index( + ctx_, group, i, &uri_ptr, &type, &name_ptr); REQUIRE(rc == TILEDB_OK); + uri = uri_ptr == nullptr ? "" : std::string(uri_ptr); } + ret.emplace_back(uri, type); } return ret; } From 3f73c58d8327cd0d8d2ea6aac4684b1813e894f2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 10:39:42 +0300 Subject: [PATCH 11/27] Update the test. Use sections instead of GENERATE, and move them outside the loop. And document that `tiledb_group_get_member_by_index` leaks memory. --- test/src/unit-capi-group.cc | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index 4f42b33fdc6..caad34af08f 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -235,13 +235,10 @@ std::vector> GroupFx::read_group( uint64_t count = 0; int rc = tiledb_group_get_member_count(ctx_, group, &count); REQUIRE(rc == TILEDB_OK); - for (uint64_t i = 0; i < count; i++) { - std::string uri; - tiledb_object_t type; - // TODO: When tiledb_group_get_member_by_index gets removed, replace the - // following value generation and if with the statement-true of the if. - bool use_v2 = GENERATE(true, false); - if (use_v2) { + SECTION("Using tiledb_group_get_member_by_index_v2") { + for (uint64_t i = 0; i < count; i++) { + std::string uri; + tiledb_object_t type; tiledb_string_t *uri_handle, *name_handle; rc = tiledb_group_get_member_by_index_v2( ctx_, group, i, &uri_handle, &type, &name_handle); @@ -257,14 +254,23 @@ std::vector> GroupFx::read_group( rc = tiledb_string_free(&name_handle); CHECK(rc == TILEDB_OK); } - } else { + ret.emplace_back(uri, type); + } + } + // When tiledb_group_get_member_by_index gets removed, this section can simply + // be removed. + SECTION("Using tiledb_group_get_member_by_index") { + for (uint64_t i = 0; i < count; i++) { + std::string uri; + tiledb_object_t type; char *uri_ptr, *name_ptr; + // NOTE: The following function leaks memory. rc = tiledb_group_get_member_by_index( ctx_, group, i, &uri_ptr, &type, &name_ptr); REQUIRE(rc == TILEDB_OK); uri = uri_ptr == nullptr ? "" : std::string(uri_ptr); + ret.emplace_back(uri, type); } - ret.emplace_back(uri, type); } return ret; } From 5f482041f691304175f6ea762be5c47cade31ca7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 10:59:13 +0300 Subject: [PATCH 12/27] Update `CAPIStringHandle`. It now cannot contain a null handle, and its helper methods were moved outside the class. --- tiledb/sm/cpp_api/capi_string.h | 48 ++++++++++++-------------- tiledb/sm/cpp_api/fragment_info.h | 2 +- tiledb/sm/cpp_api/group_experimental.h | 6 ++-- 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 0af285bf5a6..96701227379 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -46,7 +46,10 @@ namespace tiledb::impl { */ class CAPIString { public: - CAPIString(tiledb_string_t*&& handle) noexcept { + CAPIString(tiledb_string_t*&& handle) { + if (handle == nullptr) { + throw std::invalid_argument("String handle cannot be null."); + } string_ = handle; handle = nullptr; } @@ -61,48 +64,41 @@ class CAPIString { } } - // Disable copy and move. + // Disable copy and move. Because this class owns a resource, + // copying it must not be supported, but moving it could be. CAPIString(const CAPIString&) = delete; CAPIString operator=(const CAPIString&) = delete; CAPIString(const CAPIString&&) = delete; CAPIString operator=(const CAPIString&&) = delete; - /** - * Returns a C++ string with the handle's data. - * - * If the handle is null returns nullopt. - */ - static std::optional to_string_optional( - tiledb_string_t*&& handle) { - if (handle == nullptr) { - return std::nullopt; - } - return to_string(std::move(handle)); - } - - /** - * Returns a C++ string with the handle's data. - */ - static std::string to_string(tiledb_string_t*&& handle) { - return CAPIString(std::move(handle)).str(); - } - std::string str() const { const char* c; size_t size; - capi_status_t status = - tiledb_status(tiledb_string_view(string_, &c, &size)); + auto status = tiledb_status(tiledb_string_view(string_, &c, &size)); if (status != TILEDB_OK) { - throw std::runtime_error( + throw TileDBError( "Could not view string; Error code: " + std::to_string(status)); } - return std::string(c, size); + return {c, size}; } private: + /** The C API string handle. Invariant: must not be null. */ tiledb_string_t* string_; }; +/** + * Returns a C++ string with the handle's data. The handle is subsequently freed. + * + * If the handle is null returns nullopt. + */ +inline std::optional make_CAPIString(tiledb_string_t*&& handle) { + if (handle == nullptr) { + return std::nullopt; + } + return CAPIString(std::move(handle)).str(); +} + } // namespace tiledb::impl #endif // TILEDB_CPP_API_CAPI_STRING_H diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index 9ae2735e02b..2e250528da7 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -98,7 +98,7 @@ class FragmentInfo { tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( ctx.ptr().get(), fragment_info_.get(), fid, &name)); - return impl::CAPIString::to_string(std::move(name)); + return impl::make_CAPIString(std::move(name)).value(); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index ca77485a028..bd6b2a1a30d 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -421,8 +421,8 @@ class Group { c_ctx, group_.get(), index, &uri, &type, &name)); return tiledb::Object( type, - impl::CAPIString::to_string(std::move(uri)), - impl::CAPIString::to_string_optional(std::move(name))); + impl::make_CAPIString(std::move(uri)).value(), + impl::make_CAPIString(std::move(name))); } tiledb::Object member(std::string name) const { @@ -433,7 +433,7 @@ class Group { ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); return tiledb::Object( - type, impl::CAPIString::to_string(std::move(uri)), name); + type, impl::make_CAPIString(std::move(uri)).value(), name); } /** From 4660ebd03029b6ff1dfe9a4785509a8216e1d319 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 11:10:15 +0300 Subject: [PATCH 13/27] Use `auto` once more. --- tiledb/sm/cpp_api/capi_string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 96701227379..c9c13c1f6e2 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -58,7 +58,7 @@ class CAPIString { return; } - capi_status_t result = tiledb_status(tiledb_string_free(&string_)); + auto result = tiledb_status(tiledb_string_free(&string_)); if (result != TILEDB_OK) { log_warn("Could not free string; Error code: " + std::to_string(result)); } From 948ca027559e326c7aa6465fd6866b557804bbc9 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 11:15:43 +0300 Subject: [PATCH 14/27] Rename `make_CAPIString` to `to_string`. --- tiledb/sm/cpp_api/capi_string.h | 5 +++-- tiledb/sm/cpp_api/fragment_info.h | 2 +- tiledb/sm/cpp_api/group_experimental.h | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index c9c13c1f6e2..1c2026320ae 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -88,11 +88,12 @@ class CAPIString { }; /** - * Returns a C++ string with the handle's data. The handle is subsequently freed. + * Returns a C++ string with the handle's data. The handle is subsequently + * freed. * * If the handle is null returns nullopt. */ -inline std::optional make_CAPIString(tiledb_string_t*&& handle) { +inline std::optional to_string(tiledb_string_t*&& handle) { if (handle == nullptr) { return std::nullopt; } diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index 2e250528da7..ce16826bbf5 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -98,7 +98,7 @@ class FragmentInfo { tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( ctx.ptr().get(), fragment_info_.get(), fid, &name)); - return impl::make_CAPIString(std::move(name)).value(); + return impl::to_string(std::move(name)).value(); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index bd6b2a1a30d..f9bc5930b6e 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -421,8 +421,8 @@ class Group { c_ctx, group_.get(), index, &uri, &type, &name)); return tiledb::Object( type, - impl::make_CAPIString(std::move(uri)).value(), - impl::make_CAPIString(std::move(name))); + impl::to_string(std::move(uri)).value(), + impl::to_string(std::move(name))); } tiledb::Object member(std::string name) const { @@ -433,7 +433,7 @@ class Group { ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); return tiledb::Object( - type, impl::make_CAPIString(std::move(uri)).value(), name); + type, impl::to_string(std::move(uri)).value(), name); } /** From 3920281cafb51333d4e658dfa7ee4ce18a0f72cc Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 11:17:28 +0300 Subject: [PATCH 15/27] Fix formatting. --- tiledb/sm/cpp_api/group_experimental.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index f9bc5930b6e..0df2cd815ca 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -432,8 +432,7 @@ class Group { tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); - return tiledb::Object( - type, impl::to_string(std::move(uri)).value(), name); + return tiledb::Object(type, impl::to_string(std::move(uri)).value(), name); } /** From 35038c6f8e84d631b68c43a656ead944f1fc1f26 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 2 May 2023 19:08:20 +0300 Subject: [PATCH 16/27] Do not use `SECTION`s inside `GroupFx::read_group`. Whether that method will use the old or new API depends on a new parameter that defaults to the new. As a consequence only one test will use the old method, to avoid copy-pasting the same boilerplate everywhere (we don't need full coverage for a deprecated function). --- test/src/unit-capi-group.cc | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index caad34af08f..a94c34df72e 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -86,7 +86,7 @@ struct GroupFx { std::string get_golden_ls(const std::string& path); static std::string random_name(const std::string& prefix); std::vector> read_group( - tiledb_group_t* group) const; + tiledb_group_t* group, bool use_get_member_by_index_v2 = true) const; void set_group_timestamp( tiledb_group_t* group, const uint64_t& timestamp) const; void write_group_metadata(const char* group_uri) const; @@ -230,12 +230,12 @@ void GroupFx::vacuum(const char* group_uri) const { } std::vector> GroupFx::read_group( - tiledb_group_t* group) const { + tiledb_group_t* group, bool use_get_member_by_index_v2) const { std::vector> ret; uint64_t count = 0; int rc = tiledb_group_get_member_count(ctx_, group, &count); REQUIRE(rc == TILEDB_OK); - SECTION("Using tiledb_group_get_member_by_index_v2") { + if (use_get_member_by_index_v2) { for (uint64_t i = 0; i < count; i++) { std::string uri; tiledb_object_t type; @@ -256,10 +256,9 @@ std::vector> GroupFx::read_group( } ret.emplace_back(uri, type); } - } - // When tiledb_group_get_member_by_index gets removed, this section can simply - // be removed. - SECTION("Using tiledb_group_get_member_by_index") { + } else { + // When tiledb_group_get_member_by_index gets removed, the else part can + // simply be removed. for (uint64_t i = 0; i < count; i++) { std::string uri; tiledb_object_t type; @@ -521,6 +520,13 @@ TEST_CASE_METHOD( TEST_CASE_METHOD( GroupFx, "C API: Group, write/read", "[capi][group][metadata][read]") { + bool use_get_member_by_index_v2 = true; + SECTION("Using tiledb_group_get_member_by_index_v2") { + use_get_member_by_index_v2 = true; + } + SECTION("Using tiledb_group_get_member_by_index") { + use_get_member_by_index_v2 = false; + } // Create and open group in write mode // TODO: refactor for each supported FS. std::string temp_dir = fs_vec_[0]->temp_dir(); @@ -590,12 +596,12 @@ TEST_CASE_METHOD( REQUIRE(rc == TILEDB_OK); std::vector> group1_received = - read_group(group1); + read_group(group1, use_get_member_by_index_v2); REQUIRE_THAT( group1_received, Catch::Matchers::UnorderedEquals(group1_expected)); std::vector> group2_received = - read_group(group2); + read_group(group2, use_get_member_by_index_v2); REQUIRE_THAT( group2_received, Catch::Matchers::UnorderedEquals(group2_expected)); From f538bdadbc4b27706ef7ad016783e93ec45fd054 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 4 Oct 2023 15:52:46 +0300 Subject: [PATCH 17/27] Stop using rvalue references, according to review feedback. --- tiledb/sm/cpp_api/capi_string.h | 10 ++++++---- tiledb/sm/cpp_api/group_experimental.h | 6 ++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 1c2026320ae..75f5407088b 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -36,6 +36,8 @@ #include #include #include +#include "exception.h" +#include "log.h" #include "tiledb.h" namespace tiledb::impl { @@ -46,11 +48,11 @@ namespace tiledb::impl { */ class CAPIString { public: - CAPIString(tiledb_string_t*&& handle) { + CAPIString(tiledb_string_t** handle) { if (handle == nullptr) { throw std::invalid_argument("String handle cannot be null."); } - string_ = handle; + string_ = *handle; handle = nullptr; } ~CAPIString() { @@ -93,11 +95,11 @@ class CAPIString { * * If the handle is null returns nullopt. */ -inline std::optional to_string(tiledb_string_t*&& handle) { +inline std::optional to_string(tiledb_string_t** handle) { if (handle == nullptr) { return std::nullopt; } - return CAPIString(std::move(handle)).str(); + return CAPIString(handle).str(); } } // namespace tiledb::impl diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index 23c6034c1db..175147de011 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -429,9 +429,7 @@ class Group { ctx.handle_error(tiledb_group_get_member_by_index_v2( c_ctx, group_.get(), index, &uri, &type, &name)); return tiledb::Object( - type, - impl::to_string(std::move(uri)).value(), - impl::to_string(std::move(name))); + type, impl::to_string(&uri).value(), impl::to_string(&name)); } tiledb::Object member(std::string name) const { @@ -441,7 +439,7 @@ class Group { tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); - return tiledb::Object(type, impl::to_string(std::move(uri)).value(), name); + return tiledb::Object(type, impl::to_string(&uri).value(), name); } /** From d266ee7d15936568e0ebdba42c3dd856b70116b3 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 4 Oct 2023 20:26:09 +0300 Subject: [PATCH 18/27] Fix compile errors. --- tiledb/sm/cpp_api/fragment_info.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index ce16826bbf5..3f322d6797d 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -98,7 +98,7 @@ class FragmentInfo { tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( ctx.ptr().get(), fragment_info_.get(), fid, &name)); - return impl::to_string(std::move(name)).value(); + return impl::to_string(&name).value(); } /** From 308f7f9c59e3491805203c5e194feae657da5887 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 4 Oct 2023 21:58:56 +0300 Subject: [PATCH 19/27] Fix typo. --- tiledb/sm/cpp_api/capi_string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 75f5407088b..c4a6cf3b75f 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -96,7 +96,7 @@ class CAPIString { * If the handle is null returns nullopt. */ inline std::optional to_string(tiledb_string_t** handle) { - if (handle == nullptr) { + if (*handle == nullptr) { return std::nullopt; } return CAPIString(handle).str(); From 7d3450f6cc0c9a6c4775d9f8fea64c71ea1a13c9 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 10 Oct 2023 00:36:00 +0300 Subject: [PATCH 20/27] Don't leak memory when testing the old APIs. --- test/src/unit-capi-group.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index 49bac931e75..745310b0db8 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -263,11 +263,14 @@ std::vector> GroupFx::read_group( std::string uri; tiledb_object_t type; char *uri_ptr, *name_ptr; - // NOTE: The following function leaks memory. rc = tiledb_group_get_member_by_index( ctx_, group, i, &uri_ptr, &type, &name_ptr); REQUIRE(rc == TILEDB_OK); uri = uri_ptr == nullptr ? "" : std::string(uri_ptr); + std::free(uri_ptr); + if (name_ptr) { + std::free(name_ptr); + } ret.emplace_back(uri, type); } } From f4be46e61a6be7f9e3c45eef9a4138349aa070f2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 19 Oct 2023 23:12:53 +0300 Subject: [PATCH 21/27] Address PR feedback. --- doc/policy/api_changes.md | 2 +- tiledb/api/CMakeLists.txt | 1 + tiledb/api/cpp_api_support/CMakeLists.txt | 29 ++++++++++++ .../api/cpp_api_support/test/CMakeLists.txt | 32 +++++++++++++ .../test/unit_cppapi_string.cc | 45 +++++++++++++++++++ tiledb/sm/cpp_api/capi_string.h | 10 ++--- tiledb/sm/cpp_api/fragment_info.h | 2 +- tiledb/sm/cpp_api/group_experimental.h | 6 ++- 8 files changed, 116 insertions(+), 11 deletions(-) create mode 100644 tiledb/api/cpp_api_support/CMakeLists.txt create mode 100644 tiledb/api/cpp_api_support/test/CMakeLists.txt create mode 100644 tiledb/api/cpp_api_support/test/unit_cppapi_string.cc diff --git a/doc/policy/api_changes.md b/doc/policy/api_changes.md index 7e36268a78c..25c689d5221 100644 --- a/doc/policy/api_changes.md +++ b/doc/policy/api_changes.md @@ -74,7 +74,7 @@ Function removal shall be announced one release cycle before removal, following - `tiledb_query_submit_async` -### 2.15.0..2.16.0 +### 2.18.0..2.19.0 - `tiledb_group_get_member_by_index` - `tiledb_group_get_member_by_name` diff --git a/tiledb/api/CMakeLists.txt b/tiledb/api/CMakeLists.txt index a0d6c8911c7..54c307cb911 100644 --- a/tiledb/api/CMakeLists.txt +++ b/tiledb/api/CMakeLists.txt @@ -29,6 +29,7 @@ include(common-root) add_subdirectory(c_api) add_subdirectory(c_api_support) add_subdirectory(c_api_test_support) +add_subdirectory(cpp_api_support) get_gathered(API_SOURCES) set(TILEDB_API_SOURCES ${API_SOURCES} PARENT_SCOPE) diff --git a/tiledb/api/cpp_api_support/CMakeLists.txt b/tiledb/api/cpp_api_support/CMakeLists.txt new file mode 100644 index 00000000000..b8a0972697c --- /dev/null +++ b/tiledb/api/cpp_api_support/CMakeLists.txt @@ -0,0 +1,29 @@ +# +# tiledb/api/cpp_api_support/CMakeLists.txt +# +# The MIT License +# +# Copyright (c) 2023 TileDB, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +# + +include(common NO_POLICY_SCOPE) + +add_test_subdirectory() diff --git a/tiledb/api/cpp_api_support/test/CMakeLists.txt b/tiledb/api/cpp_api_support/test/CMakeLists.txt new file mode 100644 index 00000000000..30acaadf619 --- /dev/null +++ b/tiledb/api/cpp_api_support/test/CMakeLists.txt @@ -0,0 +1,32 @@ +# +# tiledb/api/cpp_api_support/test/CMakeLists.txt +# +# The MIT License +# +# Copyright (c) 2023 TileDB, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +# + +include(unit_test) + +commence(unit_test cppapi_string) + this_target_sources(unit_cppapi_string.cc) + this_target_object_libraries(capi_string) +conclude(unit_test) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc new file mode 100644 index 00000000000..0f8695f6ef2 --- /dev/null +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -0,0 +1,45 @@ +/** + * @file tiledb/api/cpp_api_support/test/unit_cppapi_string.cc + * + * @section LICENSE + * + * The MIT License + * + * @copyright Copyright (c) 2023 TileDB, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * @section DESCRIPTION + * + * This file defines tests for the CAPIString class of the TileDB C++ API. + */ + +#include + +#include "tiledb/sm/cpp_api/capi_string.h" + +using namespace tiledb::impl; + +TEST_CASE( + "CAPIString: Test constructor with null parameter throws", + "[capi_string][null-param]") { + REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument); + tiledb_string_t* string = nullptr; + REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument); +} diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index c4a6cf3b75f..23bc450ffe0 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -49,17 +49,13 @@ namespace tiledb::impl { class CAPIString { public: CAPIString(tiledb_string_t** handle) { - if (handle == nullptr) { + if (handle == nullptr || *handle == nullptr) { throw std::invalid_argument("String handle cannot be null."); } string_ = *handle; - handle = nullptr; + *handle = nullptr; } ~CAPIString() { - if (string_ == nullptr) { - return; - } - auto result = tiledb_status(tiledb_string_free(&string_)); if (result != TILEDB_OK) { log_warn("Could not free string; Error code: " + std::to_string(result)); @@ -95,7 +91,7 @@ class CAPIString { * * If the handle is null returns nullopt. */ -inline std::optional to_string(tiledb_string_t** handle) { +inline std::optional convert_to_string(tiledb_string_t** handle) { if (*handle == nullptr) { return std::nullopt; } diff --git a/tiledb/sm/cpp_api/fragment_info.h b/tiledb/sm/cpp_api/fragment_info.h index 3f322d6797d..1e254657fdb 100644 --- a/tiledb/sm/cpp_api/fragment_info.h +++ b/tiledb/sm/cpp_api/fragment_info.h @@ -98,7 +98,7 @@ class FragmentInfo { tiledb_string_t* name; ctx.handle_error(tiledb_fragment_info_get_fragment_name_v2( ctx.ptr().get(), fragment_info_.get(), fid, &name)); - return impl::to_string(&name).value(); + return impl::convert_to_string(&name).value(); } /** diff --git a/tiledb/sm/cpp_api/group_experimental.h b/tiledb/sm/cpp_api/group_experimental.h index c017ade2a19..daae9387a2a 100644 --- a/tiledb/sm/cpp_api/group_experimental.h +++ b/tiledb/sm/cpp_api/group_experimental.h @@ -431,7 +431,9 @@ class Group { ctx.handle_error(tiledb_group_get_member_by_index_v2( c_ctx, group_.get(), index, &uri, &type, &name)); return tiledb::Object( - type, impl::to_string(&uri).value(), impl::to_string(&name)); + type, + impl::convert_to_string(&uri).value(), + impl::convert_to_string(&name)); } tiledb::Object member(std::string name) const { @@ -441,7 +443,7 @@ class Group { tiledb_object_t type; ctx.handle_error(tiledb_group_get_member_by_name_v2( c_ctx, group_.get(), name.c_str(), &uri, &type)); - return tiledb::Object(type, impl::to_string(&uri).value(), name); + return tiledb::Object(type, impl::convert_to_string(&uri).value(), name); } /** From 7b7057614cffaf3acf0b1b63a476a5f6725d45c4 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 24 Oct 2023 18:53:23 +0300 Subject: [PATCH 22/27] Address most PR feedback. --- tiledb/api/cpp_api_support/test/unit_cppapi_string.cc | 8 +++++++- tiledb/sm/cpp_api/capi_string.h | 9 ++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc index 0f8695f6ef2..c8d8af06cd1 100644 --- a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -30,7 +30,7 @@ * This file defines tests for the CAPIString class of the TileDB C++ API. */ -#include +#include #include "tiledb/sm/cpp_api/capi_string.h" @@ -40,6 +40,12 @@ TEST_CASE( "CAPIString: Test constructor with null parameter throws", "[capi_string][null-param]") { REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument); +} + +TEST_CASE( + "CAPIString: Test constructor with non-null parameter pointing to null " + "handle throws", + "[capi_string][null-param-ptr]") { tiledb_string_t* string = nullptr; REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument); } diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index 23bc450ffe0..f688a00569b 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -48,9 +48,16 @@ namespace tiledb::impl { */ class CAPIString { public: + /** + * Constructor. Takes ownership of the handle. + * + * @param handle A pointer to the string handle. Must not be null and must not + * point to a null handle. + */ CAPIString(tiledb_string_t** handle) { if (handle == nullptr || *handle == nullptr) { - throw std::invalid_argument("String handle cannot be null."); + throw std::invalid_argument( + "Pointer to string handle cannot be null or point to null handle."); } string_ = *handle; *handle = nullptr; From 002d98a8c68baa0aa3d6fd4c82612f146b17dc00 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 26 Oct 2023 23:01:41 +0300 Subject: [PATCH 23/27] Add more tests. --- .../test/unit_cppapi_string.cc | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc index c8d8af06cd1..b0d8e933129 100644 --- a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -32,6 +32,7 @@ #include +#include "tiledb/api/c_api/string/string_api_internal.h" #include "tiledb/sm/cpp_api/capi_string.h" using namespace tiledb::impl; @@ -49,3 +50,29 @@ TEST_CASE( tiledb_string_t* string = nullptr; REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument); } + +TEST_CASE( + "CAPIString: Test creating string handle and getting its value", + "[capi_string][get]") { + const std::string test_string = "hello"; + tiledb_string_t* handle = tiledb_string_t::make_handle(test_string); + std::string result; + + SECTION("convert_to_string") { + auto result_maybe = convert_to_string(&handle); + REQUIRE(result_maybe); + result = *result_maybe; + } + SECTION("CAPIString") { + result = CAPIString(&handle).str(); + } + + REQUIRE(handle == nullptr); + REQUIRE(result == test_string); +} + +TEST_CASE( + "CAPIString: Test convert_to_string with null handle", "[capi_string]") { + tiledb_string_t* handle = nullptr; + REQUIRE(!convert_to_string(&handle).has_value()); +} From 8c017f294cf335fbd02c416c097dc69d227f81f2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 26 Oct 2023 23:19:28 +0300 Subject: [PATCH 24/27] Test that accessing a freed string handle fails. --- tiledb/api/cpp_api_support/test/unit_cppapi_string.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc index b0d8e933129..bc8f46e6180 100644 --- a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -71,6 +71,17 @@ TEST_CASE( REQUIRE(result == test_string); } +TEST_CASE( + "CAPIString: Test that accessing freed handle fails", "[capi_string][freed_handle]") { + const std::string test_string = "hello"; + tiledb_string_t* handle = tiledb_string_t::make_handle(test_string); + tiledb_string_t* handle_copy = handle; + std::ignore = convert_to_string(&handle); + const char* chars; + uint64_t length; + REQUIRE(tiledb_string_view(handle_copy, &chars, &length) == TILEDB_ERR); +} + TEST_CASE( "CAPIString: Test convert_to_string with null handle", "[capi_string]") { tiledb_string_t* handle = nullptr; From 388a38a1e6cc727898c13be78837d785e3a996ae Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 26 Oct 2023 23:21:25 +0300 Subject: [PATCH 25/27] clang-format --- tiledb/api/cpp_api_support/test/unit_cppapi_string.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc index bc8f46e6180..ace7dc21b2d 100644 --- a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -72,7 +72,8 @@ TEST_CASE( } TEST_CASE( - "CAPIString: Test that accessing freed handle fails", "[capi_string][freed_handle]") { + "CAPIString: Test that accessing freed handle fails", + "[capi_string][freed_handle]") { const std::string test_string = "hello"; tiledb_string_t* handle = tiledb_string_t::make_handle(test_string); tiledb_string_t* handle_copy = handle; From d9c858f2128bc1692a6f88f8db5eb039e24c10f8 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 27 Oct 2023 01:29:10 +0300 Subject: [PATCH 26/27] Fix compile errors. --- tiledb/api/cpp_api_support/test/unit_cppapi_string.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc index ace7dc21b2d..ffddb3f5012 100644 --- a/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc +++ b/tiledb/api/cpp_api_support/test/unit_cppapi_string.cc @@ -78,8 +78,8 @@ TEST_CASE( tiledb_string_t* handle = tiledb_string_t::make_handle(test_string); tiledb_string_t* handle_copy = handle; std::ignore = convert_to_string(&handle); - const char* chars; - uint64_t length; + const char* chars = nullptr; + size_t length = 0; REQUIRE(tiledb_string_view(handle_copy, &chars, &length) == TILEDB_ERR); } From c3eceb112c78d72927c7ea9614e481b93f80c0b7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 1 Nov 2023 17:24:30 +0200 Subject: [PATCH 27/27] Address remaining feedback. --- tiledb/api/c_api/group/group_api.cc | 10 ++++++---- tiledb/sm/cpp_api/capi_string.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index e22468b691a..4d6e374bcb1 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -773,13 +773,14 @@ CAPI_INTERFACE( ctx, group, index, uri, type, name); } -capi_return_t tiledb_group_get_member_by_index_v2( +CAPI_INTERFACE( + group_get_member_by_index_v2, tiledb_ctx_t* ctx, tiledb_group_t* group, uint64_t index, tiledb_string_t** uri, tiledb_object_t* type, - tiledb_string_t** name) TILEDB_NOEXCEPT { + tiledb_string_t** name) { return api_entry_context( ctx, group, index, uri, type, name); } @@ -795,12 +796,13 @@ CAPI_INTERFACE( ctx, group, name, uri, type); } -capi_return_t tiledb_group_get_member_by_name_v2( +CAPI_INTERFACE( + group_get_member_by_name_v2, tiledb_ctx_t* ctx, tiledb_group_t* group, const char* name, tiledb_string_t** uri, - tiledb_object_t* type) TILEDB_NOEXCEPT { + tiledb_object_t* type) { return api_entry_context( ctx, group, name, uri, type); } diff --git a/tiledb/sm/cpp_api/capi_string.h b/tiledb/sm/cpp_api/capi_string.h index f688a00569b..bd8ffa46768 100644 --- a/tiledb/sm/cpp_api/capi_string.h +++ b/tiledb/sm/cpp_api/capi_string.h @@ -62,6 +62,7 @@ class CAPIString { string_ = *handle; *handle = nullptr; } + ~CAPIString() { auto result = tiledb_status(tiledb_string_free(&string_)); if (result != TILEDB_OK) {