Profile C, C++ APIs: alloc, free, get_name, get_homedir. #5492

bekadavis9 · 2025-04-04T21:34:05Z

Initial Profile C and C++ API implementation phases.

C API:
Defines the internal structure of a tiledb_profile_handle_t object, as well as methods tiledb_profile_alloc, tiledb_profile_free, tiledb_profile_get_name, and tiledb_profile_get_homedir. The object ordinary_profile_t has also been defined, wrapping the in-test allocator, for use in future unit tests.

C++ API:
Defines the internal structure of a TileDB::Profile object, as well as methods static free, get_name, and get_homedir. Unit test test-cppapi-profile.cc has been defined, for future testing as the class expands.

TYPE: C_API | CPP_API
DESC: Profile C, C++ APIs: alloc, free, get_name, get_homedir.

[sc-64982]

tiledb/api/c_api/rest/rest_profile_api_external.h

tiledb/api/c_api/rest/rest_profile_api.cc

tiledb/api/c_api/rest/test/unit_capi_rest_profile.cc

tiledb/api/c_api/rest/rest_profile_api_external.h

tiledb/api/c_api/rest/rest_profile_api.cc

tiledb/api/c_api/rest/rest_profile_api_external.h

tiledb/api/c_api/rest/test/compile_capi_rest_main.cc

tiledb/api/c_api/rest/test/unit_capi_rest_profile.cc

rroelke · 2025-04-09T15:27:15Z

tiledb/api/c_api_test_support/testsupport_capi_rest_profile.h

+  }
+};
+
+struct ordinary_rest_profile {


The RAII here looks pretty much like what the CPP API would eventually do. Instead of a test support class perhaps this should just be a class in the CPP API?

This is a test support class for the C API test. It will eventually be used such that an ordinary_rest_profile can be initialized at the top of a test case, and the internal tiledb_rest_profile_alloc and tiledb_rest_profile_free logic is handled for the duration of that test case. This is on par with most other C API objects' unit tests.

I had made the same question back when the ordinary_ classes were introduced (#5225 (comment)) and did not agree with the conclusion that the C and C++ API have to be tested separately in all cases, considering that the C++ API is itself a very thin layer over the C API and the ordinary_ classes are duplicate code.

We need a few tests to directly call the C API for testing argument validation edge cases, but I don't see any problem at all with using the C++ API¹ for our functional tests.

Footnotes

or even another language if hypothetically all language APIs were in the same repo ↩

tiledb/sm/rest/rest_profile.cc

bekadavis9 · 2025-04-11T18:23:47Z

test/src/unit-cppapi-profile.cc

+/**
+ * Notes to reviewer:
+ * This test is segfaulting. I suspect it's something to do with the
+ * destruction of the underlying object. I had a lot of trouble getting
+ * the behavior on L91 of tiledb/sm/cpp_api/profile_experimental.h to work,
+ * which may be a conrtibuting factor.
+ *
+ * Please see my other notes to reviewer on L127 of
+ * tiledb/sm/rest/rest_profile.h for important information regarding the
+ * get_homedir() method.
+ */


Highlighting

Which part of this is giving the SEGV? The section that is not commented out, or one of the sections that is?

bekadavis9 · 2025-04-11T18:24:31Z

tiledb/api/c_api/profile/test/unit_capi_profile.cc

+    const char* homedir;
+    rc = tiledb_profile_get_homedir(profile, &homedir);
+    REQUIRE(tiledb_status(rc) == TILEDB_OK);
+    // Validate homedir is non-empty. The path may not be resolved when using
+    // the default homedir (per the RestProfile::homedir() invariant).
+    // #TODO REQUIRE(homedir[0] != '\0');


Highlighting

bekadavis9 · 2025-04-11T18:24:51Z

tiledb/sm/rest/rest_profile.h

+   * @invariant Use of home_directory() may cause unexpected behavior
+   * from this method. Depending on where in the stack the method is invoked,
+   * the path may be unresolved.
+   *
+   * #TODO Note to reviewer: we probably need to just change the constuctor to
+   * take in a std::string_view to ensure the home_directory() path ref doesn't
+   * go out of scope. We may also decide to just scrap this method entirely.
+   * It's not in the original design but I thought it useful in prod.


Highlighting

I don't understand what going out of scope means here. Don't we store homedir_ by value? And what do you mean by the path being unresolved?

teo-tsirpanis

Left some comments. Please also add a tiledb_error_t* parameter at the end of every new C API except free.

tiledb/sm/cpp_api/profile_experimental.h

test/src/unit-cppapi-profile.cc

tiledb/sm/cpp_api/profile_experimental.h

tiledb/api/c_api/profile/profile_api_experimental.h

tiledb/sm/rest/rest_profile.cc

tiledb/sm/cpp_api/profile_experimental.h

teo-tsirpanis · 2025-04-11T18:54:06Z

tiledb/sm/rest/rest_profile.h

+   * @invariant Use of home_directory() may cause unexpected behavior
+   * from this method. Depending on where in the stack the method is invoked,
+   * the path may be unresolved.
+   *
+   * #TODO Note to reviewer: we probably need to just change the constuctor to
+   * take in a std::string_view to ensure the home_directory() path ref doesn't
+   * go out of scope. We may also decide to just scrap this method entirely.
+   * It's not in the original design but I thought it useful in prod.


I don't understand what going out of scope means here. Don't we store homedir_ by value? And what do you mean by the path being unresolved?

teo-tsirpanis · 2025-04-15T18:02:31Z

tiledb/sm/rest/rest_profile.h

+   *
+   * @return std::string The path to the local $HOME directory.
+   */
+  inline std::string homedir() {


Suggested change

inline std::string homedir() {

inline const std::string& homedir() const {

std::string_view seems like a better choice. Agree with const.

teo-tsirpanis · 2025-04-15T18:02:42Z

tiledb/sm/rest/rest_profile.h

+   * Returns the name of this profile.
+   *
+   * @return std::string The name of this profile.
+   */
  inline std::string name() {


Suggested change

inline std::string name() {

inline const std::string& name() const {

teo-tsirpanis · 2025-04-15T18:03:17Z

tiledb/sm/rest/rest_profile.cc

@@ -139,7 +145,7 @@ RestProfile::RestProfile(const std::string& name) {
   * or perhaps stop using `sudo`.
   */
  auto homedir = home_directory();
-  if (homedir.empty()) {
+  if (homedir[0] == '\0') {


Why was this changed? The old version looks better.

If homedir were const char * this would avoid a std::string construction, but as far as I can tell homedir is already a std::string here? In which case I agree empty() would be better.

tiledb/api/c_api/profile/test/compile_capi_profile_main.cc

RestProfile C API: alloc, free.

1af9b3f

bekadavis9 requested review from rroelke, teo-tsirpanis and kounelisagis April 4, 2025 21:34

teo-tsirpanis reviewed Apr 4, 2025

View reviewed changes

tiledb/api/c_api/rest/rest_profile_api_external.h Outdated Show resolved Hide resolved

ypatia reviewed Apr 7, 2025

View reviewed changes

tiledb/api/c_api/rest/rest_profile_api.cc Outdated Show resolved Hide resolved

tiledb/api/c_api/rest/test/unit_capi_rest_profile.cc Outdated Show resolved Hide resolved

teo-tsirpanis reviewed Apr 7, 2025

View reviewed changes

tiledb/api/c_api/rest/rest_profile_api_external.h Outdated Show resolved Hide resolved

tiledb/api/c_api/rest/rest_profile_api_external.h Outdated Show resolved Hide resolved

ypatia reviewed Apr 8, 2025

View reviewed changes

tiledb/api/c_api/rest/rest_profile_api_external.h Outdated Show resolved Hide resolved

rroelke requested changes Apr 9, 2025

View reviewed changes

bekadavis9 added 3 commits April 9, 2025 18:32

Address comments.

30d7156

Rename C API rest_profile -> profile.

32895d4

Add Profile C++ API.

54d2616

bekadavis9 commented Apr 11, 2025

View reviewed changes

bekadavis9 changed the title ~~RestProfile C API: alloc, free.~~ RestProfile C, C++ APIs: alloc, free, get_name. Apr 11, 2025

bekadavis9 changed the title ~~RestProfile C, C++ APIs: alloc, free, get_name.~~ Profile C, C++ APIs: alloc, free, get_name, get_homedir. Apr 11, 2025

teo-tsirpanis reviewed Apr 11, 2025

View reviewed changes

Address some comments

35b0c66

teo-tsirpanis reviewed Apr 15, 2025

View reviewed changes

Update tiledb/api/c_api/profile/test/compile_capi_profile_main.cc

4084c6a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profile C, C++ APIs: alloc, free, get_name, get_homedir. #5492

Profile C, C++ APIs: alloc, free, get_name, get_homedir. #5492

bekadavis9 commented Apr 4, 2025 •

edited

Loading

rroelke Apr 9, 2025

bekadavis9 Apr 9, 2025

teo-tsirpanis Apr 10, 2025

bekadavis9 Apr 11, 2025

rroelke Apr 16, 2025

bekadavis9 Apr 11, 2025

bekadavis9 Apr 11, 2025

teo-tsirpanis Apr 11, 2025

teo-tsirpanis left a comment

teo-tsirpanis Apr 11, 2025

teo-tsirpanis Apr 15, 2025

rroelke Apr 16, 2025 •

edited

Loading

teo-tsirpanis Apr 15, 2025

teo-tsirpanis Apr 15, 2025

rroelke Apr 16, 2025 •

edited

Loading

	inline std::string homedir() {
	inline const std::string& homedir() const {

	inline std::string name() {
	inline const std::string& name() const {

Profile C, C++ APIs: alloc, free, get_name, get_homedir. #5492

Are you sure you want to change the base?

Profile C, C++ APIs: alloc, free, get_name, get_homedir. #5492

Conversation

bekadavis9 commented Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rroelke Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rroelke Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

bekadavis9 commented Apr 4, 2025 •

edited

Loading

rroelke Apr 16, 2025 •

edited

Loading

rroelke Apr 16, 2025 •

edited

Loading