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

Convert C API interface functions to CAPI_INTERFACE #4460

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

eric-hughes-tiledb
Copy link
Contributor

Convert C API interface functions to use CAPI_INTERFACE. This completes the work in #4430.

There are a very small number of functions not converted. One is not in the C API (i.e. it doesn't have extern "C" linkage), but is used by the C++ API, and on top of that it's for a deprecated operation. The others are small, informational functions that do return values directly and do not use capi_return_t.


TYPE: NO_HISTORY
DESC: Convert C API interface functions to CAPI_INTERFACE

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 pulled this locally to give it a whirl and found a few minor issues that are tagged with comments.

The only other thing I see is that we're also not wrapping the APIs defined in tiledb_filestore.cc which isn't a huge issue but it seems like an oversight.

Handling unwrapped functions gracefully took a bit of extra thinking but it turned out easy enough to filter them out in the end.

tiledb/sm/c_api/tiledb.cc Outdated Show resolved Hide resolved
tiledb/sm/c_api/tiledb.cc Outdated Show resolved Hide resolved
tiledb/sm/c_api/tiledb_dimension_label.cc Show resolved Hide resolved
@davisp
Copy link
Contributor

davisp commented Oct 25, 2023

Whoops, I should have also mentioned that with those minor issues fixed this all works great so +1 soon as we fix those few issues.

@eric-hughes-tiledb eric-hughes-tiledb marked this pull request as ready for review October 25, 2023 22:05
@davisp
Copy link
Contributor

davisp commented Oct 25, 2023

Hooray! I've managed to get logging with arguments working locally.

Here's the code:

f906ad9

Here's the sample output:

https://gist.github.com/davisp/b7a457b8ae640c4f3dc6f0e9c375fe9e

(edited to update the commit sha to something with fewer includes)

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. Example logging commit compiles and runs fine with the new APIs logged.

One minor issue I noticed, having a capi_function_override.h header on the include path breaks the CAPI unit test builds due to the conflicting test provided capi_function_override.h. I don't think its a huge issue (tiledb_unit still works fine) but we'll likely want to figure out how to make that test a no-op when building with an external capi_function_override.h available.

@KiterLuc KiterLuc force-pushed the eh/convert_to_CAPI_INTERFACE branch from 828b67e to 4d90f36 Compare October 31, 2023 09:56
@KiterLuc KiterLuc merged commit aede0f1 into dev Oct 31, 2023
54 checks passed
@KiterLuc KiterLuc deleted the eh/convert_to_CAPI_INTERFACE branch October 31, 2023 11:47
github-actions bot pushed a commit that referenced this pull request Oct 31, 2023
Convert C API interface functions to use `CAPI_INTERFACE`. This
completes the work in #4430.

There are a very small number of functions not converted. One is not in
the C API (i.e. it doesn't have `extern "C"` linkage), but is used by
the C++ API, and on top of that it's for a deprecated operation. The
others are small, informational functions that do return values directly
and do not use `capi_return_t`.

---
TYPE: NO_HISTORY
DESC: Convert C API interface functions to CAPI_INTERFACE

(cherry picked from commit aede0f1)
KiterLuc pushed a commit that referenced this pull request Nov 1, 2023
…ERFACE (#4471)

Backport
aede0f1~5..aede0f1
from #4460.

---------

Co-authored-by: Theodore Tsirpanis <[email protected]>
Co-authored-by: Abigale Kim <[email protected]>
Co-authored-by: Robert Bindar <[email protected]>
Co-authored-by: eric-hughes-tiledb <[email protected]>
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.

3 participants