-
Notifications
You must be signed in to change notification settings - Fork 97
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
refactor(c/driver/shared): created shared util library for drivers #582
Conversation
c/cmake_modules/AdbcDefines.cmake
Outdated
@@ -69,6 +69,15 @@ set_target_properties(nanoarrow | |||
"${REPOSITORY_ROOT}/c/vendor/" | |||
POSITION_INDEPENDENT_CODE ON) | |||
|
|||
# Shared ADBC Driver libraries | |||
add_library(adbc_driver_shared STATIC EXCLUDE_FROM_ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to name this 'driver_shared' while it's a static library (usually in Arrow libraries that convention means this is a dynamic library). Maybe driver_common
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that makes more sense
Looks like R needs adjusting since it vendors the SQLite sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Trying to consolidate some of the implementations in a follow up and I noticed that StringBuilder in the postgres util implementation uses |
The SQLite utils are explicitly in C since the SQLite driver is C. |
So in that case there may be no point consolidating. |
Interesting. It would take some effort but I wonder if the common utils then shouldn't just be written in C to increase compatability. Ignoring the C/C++ distinction there are like-named macros in both util files today with different definitions; will probably be easier to align that first and worry about things like the StringBuilder C/C++ differences later |
Per @lidavidm comment #577 (comment) made this act similar to the current nanoarrow setup