Add C ABI shim for P/Invoke integration #2
Conversation
Summary of ChangesHello @username77, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the interoperability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a C ABI shim for P/Invoke integration by adding a databento_c shared library. The changes include a new C API header (c_api.h), its implementation (databento_c.cpp), and updates to CMakeLists.txt to build and install the new library and header.
The overall approach is solid. The C API is well-designed with opaque pointers for handles, clear function naming, and robust error handling using try-catch blocks and thread-local error messages. Memory management for handles across the C/C++ boundary is handled correctly.
My review focuses on improving maintainability and robustness in the C++ implementation of the shim. I've suggested making enum conversions safer, improving error messages for invalid arguments, and simplifying some of the object construction logic. These changes will make the code easier to maintain and less prone to subtle bugs in the future.
| std::vector<std::string> CopyStrings(const char* const* strings, std::size_t count) { | ||
| std::vector<std::string> result; | ||
| result.reserve(count); | ||
| for (std::size_t i = 0; i < count; ++i) { | ||
| const char* value = strings ? strings[i] : nullptr; | ||
| if (value == nullptr) { | ||
| throw std::invalid_argument("symbol list contains a null entry"); | ||
| } | ||
| result.emplace_back(value); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The current implementation of CopyStrings is a bit dense. It handles both a null strings pointer and null elements within the strings array with the same check and error message. This can be confusing for debugging.
Refactoring for clarity by handling these cases separately with more specific error messages would improve maintainability.
std::vector<std::string> CopyStrings(const char* const* strings, std::size_t count) {
if (count > 0 && !strings) {
throw std::invalid_argument("symbol list is null but count is non-zero");
}
std::vector<std::string> result;
result.reserve(count);
for (std::size_t i = 0; i < count; ++i) {
const char* value = strings[i];
if (value == nullptr) {
throw std::invalid_argument("symbol list contains a null entry");
}
result.emplace_back(value);
}
return result;
}| databento::Schema ToSchema(db_c_schema_t schema) { | ||
| return static_cast<databento::Schema>(schema); | ||
| } | ||
|
|
||
| databento::SType ToSType(db_c_stype_t stype) { | ||
| return static_cast<databento::SType>(stype); | ||
| } | ||
|
|
||
| databento::Dataset ToDataset(db_c_dataset_t dataset) { | ||
| return static_cast<databento::Dataset>(dataset); | ||
| } |
There was a problem hiding this comment.
The functions ToSchema, ToSType, and ToDataset use static_cast to convert from C enums to C++ enums. This is brittle because it silently assumes that the integer values of the corresponding enums are identical. If the enums diverge in the future, this will lead to subtle bugs.
This is also inconsistent with ToUpgradePolicy which uses a safer switch statement. For improved safety and consistency, consider using switch statements for all enum conversions. This will allow the compiler to warn about unhandled cases if the enums change.
Example for ToSchema:
databento::Schema ToSchema(db_c_schema_t schema) {
switch (schema) {
case DB_C_SCHEMA_MBO:
return databento::Schema::Mbo;
case DB_C_SCHEMA_MBP_1:
return databento::Schema::Mbp1;
// ... other cases
default:
throw std::invalid_argument("Unknown db_c_schema_t value");
}
}If a full switch is too verbose, you could add static_asserts to validate that the enum values match, which would provide compile-time safety.
| databento::VersionUpgradePolicy ToUpgradePolicy(db_c_upgrade_policy_t policy) { | ||
| switch (policy) { | ||
| case DB_C_UPGRADE_AS_IS: | ||
| return databento::VersionUpgradePolicy::AsIs; | ||
| case DB_C_UPGRADE_TO_V2: | ||
| return databento::VersionUpgradePolicy::UpgradeToV2; | ||
| case DB_C_UPGRADE_TO_V3: | ||
| default: | ||
| return databento::VersionUpgradePolicy::UpgradeToV3; | ||
| } | ||
| } |
There was a problem hiding this comment.
The switch statement in ToUpgradePolicy has a default case that falls back to UpgradeToV3. This can hide potential bugs. If a new value is added to db_c_upgrade_policy_t but the switch is not updated, it will silently be treated as UpgradeToV3 instead of causing a compile-time or runtime error.
To make this more robust, it's better to handle each enum member explicitly and have the default case throw an exception for unknown values. This ensures that any future changes to the enum must be handled here.
databento::VersionUpgradePolicy ToUpgradePolicy(db_c_upgrade_policy_t policy) {
switch (policy) {
case DB_C_UPGRADE_AS_IS:
return databento::VersionUpgradePolicy::AsIs;
case DB_C_UPGRADE_TO_V2:
return databento::VersionUpgradePolicy::UpgradeToV2;
case DB_C_UPGRADE_TO_V3:
return databento::VersionUpgradePolicy::UpgradeToV3;
default:
throw std::invalid_argument("Unknown db_c_upgrade_policy_t value");
}
}| db_c_live_threaded_t* db_c_live_builder_build_threaded(db_c_live_builder_t* builder) { | ||
| if (!builder) { | ||
| SetError("builder pointer is null"); | ||
| return nullptr; | ||
| } | ||
| try { | ||
| ClearError(); | ||
| auto native_client = builder->builder.BuildThreaded(); | ||
| auto wrapper = std::make_unique<db_c_live_threaded>(); | ||
| wrapper->client = std::make_unique<databento::LiveThreaded>(std::move(native_client)); | ||
| return wrapper.release(); | ||
| } catch (const std::exception& ex) { | ||
| SetError(ex.what()); | ||
| } catch (...) { | ||
| SetError("unexpected error in db_c_live_builder_build_threaded"); | ||
| } | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
The implementation of db_c_live_builder_build_threaded creates a LiveThreaded object on the stack, then move-constructs another one inside a std::unique_ptr. This can be simplified by constructing the LiveThreaded object directly in the std::unique_ptr from the temporary returned by builder->builder.BuildThreaded(). This avoids an extra named variable and an extra move operation, making the code more direct.
db_c_live_threaded_t* db_c_live_builder_build_threaded(
db_c_live_builder_t* builder) {
if (!builder) {
SetError("builder pointer is null");
return nullptr;
}
try {
ClearError();
auto wrapper = std::make_unique<db_c_live_threaded>();
wrapper->client = std::make_unique<databento::LiveThreaded>(
builder->builder.BuildThreaded());
return wrapper.release();
} catch (const std::exception& ex) {
SetError(ex.what());
} catch (...) {
SetError("unexpected error in db_c_live_builder_build_threaded");
}
return nullptr;
}
Summary
databento_clibrary that wrapsLiveThreadedbehind a C ABI for consumption via P/Invokedatabento/c_api.hTesting
https://chatgpt.com/codex/tasks/task_e_68f730d338dc83249d0b20f34148eb10