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

Add tiledb_group_get_member_by(index|name)_v2. #4019

Merged
merged 33 commits into from
Nov 2, 2023

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented Apr 3, 2023

SC-23227

The APIs' implementation on the Core side is very similar to #3842. Just like that PR, the old functions were left untested.

For the C++ API I created a StringHandleHolder class that manages the handles' lifetime, similar to what I have done in C#. It is easier and safer to use than the existing way of using unique pointers.


TYPE: C_API
DESC: Add new C API functions to get group members and deprecate the existing ones.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #23227: Fix tiledb_group_get_member_by_index API.

It is much easier to use and correctly handles null handles.
@teo-tsirpanis teo-tsirpanis force-pushed the group-get-member-by-index-v2 branch from dda4411 to 8dde4aa Compare April 3, 2023 16:06
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to retain tests for deprecated functions, and they need to be easily separated out for future removal.

I don't particularly like the Deleter class in this code, but largely because it's not a class that provides semantic translation to the C API in any way. Most handles aren't reused across C API sections, so there's been little need to write such translations as reusable utility provided by some class. It's perfectly fine to have a class that holds a string handle, but it should always hold such a handle, not maybe hold such a handle. Calling the class CAPIString would be perfectly descriptive. Its constructor should take a C API string handle. It should be declared noexcept to ensure that the handle gets freed.

In the case where such strings might be missing, use optional<CAPIString> rather than baking optionality into the class with poor life span management.


*uri = tiledb_string_handle_t::make_handle(uri_str);
*type = static_cast<tiledb_object_t>(object_type);
*name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_handle in 368 will leak if 369 or 370 fails. In this case, get all three return values before making any handles. The only failure to worry about then is that the first string handle will need to be broken if making the second string handle fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

std::free(name);
const char* uri_str;
size_t uri_size;
rc = tiledb_string_view(uri, &uri_str, &uri_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check the return value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be CHECK here so that the test fails but the call to free name still happens below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*/
class StringHandleHolder {
public:
StringHandleHolder() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a new class just to add a default constructor so that it can be initialized in some other is generally a bad idea. If you find yourself doing this in C++, step back and ask yourself what you're actually doing.

* Manages the lifetime of a tiledb_string_t* handle and provides operations on
* it.
*/
class StringHandleHolder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want this class, not as currently written. C++ has a different life span model that C#, and what might be appropriate there is not appropriate here.

It's biggest problem, and it's fatal, is that is doesn't follow RA=I. A class that's supposed to hold a handle should either hold a handle or not exist. There should not be some kind of partially-initialized state possible. In this case there's a two phase initialization which is (1) default construction, and (2) pass the result of c_ptr to an API function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote it. It now more explicitly "owns" the string handle. Let me know what you think.


/**
* Returns a tiledb_string_t** pointer to be passed in native code.
* This method should be called only once.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be called

A class that requires a particular calling sequence for initialization is less safe that one that adheres to C.41.

doc/policy/api_changes.md Show resolved Hide resolved

*uri = tiledb_string_handle_t::make_handle(uri_str);
*type = static_cast<tiledb_object_t>(object_type);
*name = name_str ? tiledb_string_handle_t::make_handle(*name_str) : nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should be name_str.has_value() to be clear. Relying on implicit conversion to boolean is correct, but passes over an important point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@ihnorton ihnorton requested review from eric-hughes-tiledb and removed request for eric-hughes-tiledb April 26, 2023 13:19
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly OK.

  • CAPIString needs to always hold a resource.
  • Two SECTION instead of a GENERATE

Comment on lines 243 to 244
bool use_v2 = GENERATE(true, false);
if (use_v2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be much better done with two SECTION declaration. As a further benefit, the section names can document whether they're testing the v2 or the original function.

Also, the SECTION should be outside the loop, rather than inside of it. That way the test has two sections, rather than 2n sections

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just replacing the GENERATE with sections caused test failures; looks like Catch does not support it. Instead I added a parameter in read_group that determines whether to use the old or the new API, and added the sections in the test itself (only one, don't think testing both APIs on every single test it's worth it).

test/src/unit-capi-group.cc Show resolved Hide resolved
*/
class CAPIString {
public:
CAPIString(tiledb_string_t*&& handle) noexcept {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor is trying to have move semantics, but doesn't. Assigning nullptr to handle at the end of the function does nothing, since it's a temporary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I passed the handle with a && to force callers to use std::move. And I see it having effects on a debugger.

Before

image

After

image

~CAPIString() {
if (string_ == nullptr) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a very good indicator that this isn't a C.41 class. It maybe-holds-a-string, rather than holding a string. There should be a class invariant that string_ is never nullptr. That means that a constructor should throw if there's no string to capture (and thus also not be declared noexcept).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a null check in the constructor but kept this in the destructor. If in the future we implement move constructors, the handle will become null and we don't want to free a null handle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in the future we implement move constructors

In its current code this is test for something that doesn't occur; we don't want that. We can resolve in two ways:
(1) implement the move constructor so that the test is required
(2) remove the test as useless
For now, I recommend option (2), purely out of concerns of scope creep.

tiledb/sm/cpp_api/capi_string.h Outdated Show resolved Hide resolved
return;
}

capi_status_t result = tiledb_status(tiledb_string_free(&string_));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use an auto declaration here without any ambiguity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}

// Disable copy and move.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth documenting that this class owns a resource. Thus copy construction and assignment can't be implemented, but move construction and assignment could be implemented but aren't because they're not needed. That way if anybody ever needs them, it'll be clear what to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return std::nullopt;
}
return to_string(std::move(handle));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function won't be needed at all after the class strictly holds a resource.

It might be desirable to have a factory method optional<CAPIString> make_CAPIString() noexcept. As always, keep optionality out of a resource class and explicitly use optional if you need optional behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I added a helper function std::optional<std::string> to_string(tiledb_string_t*&&).

*/
static std::string to_string(tiledb_string_t*&& handle) {
return CAPIString(std::move(handle)).str();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a member function? It doesn't look like it should be related to this class at all. If we need something to create a std:string copy from a string handle, that can be a free function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

throw std::runtime_error(
"Could not view string; Error code: " + std::to_string(status));
}
return std::string(c, size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer uniform initialization syntax: return {c, size};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Use sections instead of GENERATE, and move them outside the loop.
And document that `tiledb_group_get_member_by_index` leaks memory.
It now cannot contain a null handle, and its helper methods were moved outside the class.
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).
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issues with the majority of this. The note about a memory leak is concerning though so I'd like to understand that before approving.

test/src/unit-capi-group.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this my +1 conditional on @eric-hughes-tiledb approving given he had changes requested.

Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic design of CAPIString is sound, but it's still got a few rough edges.

There's a glaring omission that CAPIString doesn't have any unit tests. There's one test in the comments that would have found a defect. The header capi_string.h should eventually reside in tiledb/api/cpp_api_support. Putting it there now would disrupt the build beyond the scope of this PR, but a unit test runner could reside there and not risk interfering with how the C++ API headers are delivered currently.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the version numbers here.

@@ -230,20 +230,49 @@ void GroupFx::vacuum(const char* group_uri) const {
}

std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> GroupFx::read_group(
tiledb_group_t* group) const {
tiledb_group_t* group, bool use_get_member_by_index_v2) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would better have been done as a template argument. It's adequate as-is, though. Using a template argument would make removal easier, because there's so little shared code that the two versions could have been defined with specializations instead of if or even if constexpr. Ultimate removal would then entail removing a whole function definition instead of editing one.

throw std::invalid_argument("String handle cannot be null.");
}
string_ = *handle;
handle = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In present code this assignment does nothing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, a leftover from the time handle was a reference. Fixed, thanks!

~CAPIString() {
if (string_ == nullptr) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in the future we implement move constructors

In its current code this is test for something that doesn't occur; we don't want that. We can resolve in two ways:
(1) implement the move constructor so that the test is required
(2) remove the test as useless
For now, I recommend option (2), purely out of concerns of scope creep.

*
* If the handle is null returns nullopt.
*/
inline std::optional<std::string> to_string(tiledb_string_t** handle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the behavior of this function, but it really clashes with the name. The name violates the principle of Least Surprise. to_string functions are not expected to alter their arguments, but rather simply provide a text representation of them. You want something like convert_to_string.

}

private:
/** The C API string handle. Invariant: must not be null. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no enforcement of the declared invariant.

tiledb_string_t * p{nullptr};
CAPIString s{&p};

This code will pass the constructor but violate the invariant. Presumably this won't matter if (1) all string handles come from C API functions and (2) all those functions are correctly used (i.e. they check errors). Neither of these should be relied on for correct operation of this class.

This case needs a test case to ensure the constructor throws.

size_t length;
ctx.handle_error(tiledb_string_view(name_ptr.get(), &name_c, &length));
return std::string(name_c, length);
return impl::to_string(&name).value();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting this to a single line of code is a sign that you've got a good design.

}
return tiledb::Object(type, uri_str, name_opt);
return tiledb::Object(
type, impl::to_string(&uri).value(), impl::to_string(&name));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the third argument of Object take an optional?

Copy link
Member Author

@teo-tsirpanis teo-tsirpanis Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

const std::optional<std::string>& name = std::nullopt)

@teo-tsirpanis
Copy link
Member Author

Feedback addressed.

Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close to done. Need more test coverage.

* This file defines tests for the CAPIString class of the TileDB C++ API.
*/

#include <catch.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this and not tdb_catch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not know about this, thanks. I replaced the header.

"[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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be two test cases. The description does not describe the second test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the test case.

@@ -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.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two different ways for it to be invalid: a handle that's null and a non-null handle that contains null pointer. The exception message does not apply to the second, which represents an invalid handle (that's not a null handle).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded the exception message.

@@ -49,17 +49,13 @@ namespace tiledb::impl {
class CAPIString {
public:
CAPIString(tiledb_string_t** handle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor needs documentation.

The documentation needs to very clearly state that CAPIString assumes ownership of its argument and that it makes it unavailable to the caller.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would have been more apparent with handle being a tiledb_string_t*&&, but added documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the argument does not determine whether a function takes ownership of the handle. Documentation is required to specify this.

C++ specific tricks like requiring the argument to have move semantics and not available here. We are constrained to take a pointer-to-something, since that's all the C API is capable of.

REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument);
tiledb_string_t* string = nullptr;
REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There a more tests to add.

  • A check that the constructor takes ownership and zeros out the calling handle. That would have caught the constructor defect just fixed.
  • We can validate that the destructor calls tiledb_string_free:
    • Create a string. Make a copy of the handle.
    • Capture the string in CAPIString. Let it go out of scope.
    • Try to do an operation on the copied handle. It should fail.
  • A full cycle test that references str() explicitly.
  • A full cycle test that calls convert_to_string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't see much value in adding more tests for CAPIString. This class is an internal implementation detail of the C++ API that is already implicitly tested through testing the APIs that make use of it. There is no point IMHO in testing it for any additional scenario we don't actually use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more tests.

Try to do an operation on the copied handle. It should fail.

I'm not sure if it's a good idea to intentionally do a use-after-free even in a test, but did it nevertheless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already implicitly tested

Implicit testing is insufficient testing.

The point of unit testing is not merely to exercise all the code of a unit under test. It's to create a set of related-and-different tests that allow differential diagnosis of problems. One of the main kinds of deficiencies in unit tests is to have a single test that's the only test that demonstrates failure arising from multiple defects. In that situation there's no way to tell from the results of the test suite which of the multiple defects was the one that faulted. Adding tests that separate out multiple faults from each other remedies this kind of deficiency.

One other points to mention, but not elaborate here on. Tests should be written to interfaces, not to implementations. Implementations change and tests must anticipate that. In the present PR, this means testing convert_to_string without relying on the knowledge that it's implemented with CAPIString internally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentionally do a use-after-free

It's hazardous with malloc/free because those function use raw pointers and not handles (double pointers). Using a handle after it's been transferred should never be a problem since there's a null value to check against. In the case of malloc/free it's a putatively-valid value which is actually invalid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scenario we don't actually use

Those scenarios change. They expand over time. The correct criterion is "any scenario that might ever be used". It's utterly unrealistic to expect users of existing code to analyze it to see if they're using it in a novel way that doesn't have test coverage.

The only code that doesn't need testing is code that's never written in the first place. This isn't a trivial criterion. It should lead to a style of development that does less initially, but does it correctly and with assurance of correct behavior through testing. It's easier to expand the functionality of working code than to deal with untested "features" of old and broken code.

@KiterLuc KiterLuc self-requested a review October 26, 2023 15:32
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing could still be more robust, but this is adequate to proceed.

REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument);
tiledb_string_t* string = nullptr;
REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already implicitly tested

Implicit testing is insufficient testing.

The point of unit testing is not merely to exercise all the code of a unit under test. It's to create a set of related-and-different tests that allow differential diagnosis of problems. One of the main kinds of deficiencies in unit tests is to have a single test that's the only test that demonstrates failure arising from multiple defects. In that situation there's no way to tell from the results of the test suite which of the multiple defects was the one that faulted. Adding tests that separate out multiple faults from each other remedies this kind of deficiency.

One other points to mention, but not elaborate here on. Tests should be written to interfaces, not to implementations. Implementations change and tests must anticipate that. In the present PR, this means testing convert_to_string without relying on the knowledge that it's implemented with CAPIString internally.

@@ -49,17 +49,13 @@ namespace tiledb::impl {
class CAPIString {
public:
CAPIString(tiledb_string_t** handle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the argument does not determine whether a function takes ownership of the handle. Documentation is required to specify this.

C++ specific tricks like requiring the argument to have move semantics and not available here. We are constrained to take a pointer-to-something, since that's all the C API is capable of.

REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument);
tiledb_string_t* string = nullptr;
REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentionally do a use-after-free

It's hazardous with malloc/free because those function use raw pointers and not handles (double pointers). Using a handle after it's been transferred should never be a problem since there's a null value to check against. In the case of malloc/free it's a putatively-valid value which is actually invalid.

REQUIRE_THROWS_AS(CAPIString(nullptr), std::invalid_argument);
tiledb_string_t* string = nullptr;
REQUIRE_THROWS_AS(CAPIString(&string), std::invalid_argument);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scenario we don't actually use

Those scenarios change. They expand over time. The correct criterion is "any scenario that might ever be used". It's utterly unrealistic to expect users of existing code to analyze it to see if they're using it in a novel way that doesn't have test coverage.

The only code that doesn't need testing is code that's never written in the first place. This isn't a trivial criterion. It should lead to a style of development that does less initially, but does it correctly and with assurance of correct behavior through testing. It's easier to expand the functionality of working code than to deal with untested "features" of old and broken code.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good except we need to update to the new CAPI_INTERFACE macro. Beyond that it was just a style nit that apparently isn't caught by clang-format.

Comment on lines 753 to 763
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<tiledb::api::tiledb_group_get_member_by_index_v2>(
ctx, group, index, uri, type, name);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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<tiledb::api::tiledb_group_get_member_by_index_v2>(
ctx, group, index, uri, type, name);
}
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) {
return api_entry_context<tiledb::api::tiledb_group_get_member_by_index_v2>(
ctx, group, index, uri, type, name);
}

It looks like this needs to be rebased and converted to the new CAPI_INTERFACE approach.

@@ -703,6 +771,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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too on the CAPI_INTERFACE macro.

}
string_ = *handle;
*handle = nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing a blank line between functions.

@teo-tsirpanis
Copy link
Member Author

Feedback addressed.

@teo-tsirpanis teo-tsirpanis requested a review from davisp November 1, 2023 15:25
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@KiterLuc KiterLuc merged commit 89f23eb into TileDB-Inc:dev Nov 2, 2023
53 checks passed
@teo-tsirpanis teo-tsirpanis deleted the group-get-member-by-index-v2 branch November 2, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants