fix(c/driver/sqlite): Make SQLite driver C99 compliant #1946
Conversation
| 'warning_level=2', | ||
| 'cpp_std=c++17', | ||
| ] | ||
| ) | ||
|
|
||
| add_project_arguments('-Wno-int-conversion', '-Wno-unused-parameter', language: 'c') |
There was a problem hiding this comment.
Not required for this PR but these are all the warnings showing up in Meson currently. Could tackle these as follow ups to get us to a clean build
|
You should be able to |
c/driver/common/utils.c
Outdated
| if (!key_data) return; | ||
| snprintf(key_data, strlen(key), "%s", key); |
There was a problem hiding this comment.
Will give that a look tomorrow. Tried strcpy originally but cpplint did not appreciate that
c/driver/sqlite/statement_reader.c
Outdated
| #else | ||
| if (gmtime_r(&time, &broken_down_time) != &broken_down_time) { | ||
| sqlite3_mutex_enter(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MAIN)); |
There was a problem hiding this comment.
Hmm, the SQLite docs state that we're not supposed to use this mutex
There was a problem hiding this comment.
Nice catch - I just replaced this with the mutex for the sqlite3 connection, which is used elsewhere. That may be a little overkill but I figured easier to implement than a separate static mutex for the time struct
Not an expert in multithreaded C code so definitely open to feedback
WillAyd
changed the title
c/driver/sqlite/statement_reader.c
Outdated
| #else | ||
| if (gmtime_r(&time, &broken_down_time) != &broken_down_time) { | ||
| SetError(error, "Could not convert date %" PRId32 " to broken down time", value); | ||
| sqlite3_mutex_enter(sqlite3_db_mutex(conn)); |
There was a problem hiding this comment.
I guess the problem with this is if there's multiple connections in the program, this can race on gmtime since there'll be different mutexes... the easiest might be for us to just declare our own global mutex. There's still possibility for races, but well, ideally this is only a fallback and hopefully most platforms have gmtime_r?
There was a problem hiding this comment.
At least, for CMake/release builds, I'd like to use gmtime_r
There was a problem hiding this comment.
Found a potentially easier way for now to just explicitly use the feature test macro for the gmtime_r POSIX function in the sqlite code. Hope that standard is old enough by now that most ADBC compilers would be able to provide that
closes #1944
In its current state this fails cpplint, which (for arguably good reason) discourages use of gmtime in favor of gmtime_r. The macros are inspired by similar code in the vendored sqlite.c source however, so I think reasonably accommodate that concern.
Not sure if its worth trying to work around that with cpplint or just disable it for C sources