refactor(c/driver/postgresql): Remove utils duplication #628
Conversation
WillAyd
commented
Apr 28, 2023
WillAyd
commented
- Use the new SetError with variadic arguments
- Replaced StringBuilder with some std::string concatenation where applicable
- Replaced any CHECK_ macro with the variant from either nanoarrow or existing util
- Renamed util.h -> postgres_util.h to disambiguate; right now this seems just scoped to utils for network byte swapping
|
|
||
| namespace adbcpq { | ||
| AdbcStatusCode PostgresConnection::Commit(struct AdbcError* error) { | ||
| if (autocommit_) { | ||
| SetError(error, "Cannot commit when autocommit is enabled"); | ||
| SetError(error, "%s", "[libpq] Cannot commit when autocommit is enabled"); |
There was a problem hiding this comment.
Instead of hard-coding libpq or SQLite another option would be to wrap this in a macro, use __FILE__ and traverse up to the parent directory to get either postgresql or sqlite, etc...
c/driver/postgresql/database.cc
Outdated
| @@ -42,7 +42,8 @@ AdbcStatusCode PostgresDatabase::Init(struct AdbcError* error) { | |||
|
|
|||
| AdbcStatusCode PostgresDatabase::Release(struct AdbcError* error) { | |||
| if (open_connections_ != 0) { | |||
| SetError(error, "Database released with ", open_connections_, " open connections"); | |||
| SetError(error, "%s%d%s", "[libpq] Database released with ", open_connections_, | |||
There was a problem hiding this comment.
Is open_connections_ an int? I forget the details, but I think -Wpedantic on gcc will fire if it is not and 32-bit windows may segfault. I usually do static_cast<int>() for %d or static_cast<long>() for %ld to make sure since most of our C++ uses int32_t and int64_t.
There was a problem hiding this comment.
Nice catch this is a mistake
There was a problem hiding this comment.
Its an int32 so replaced with the appropriate inttypes macro
|
Kicked the R jobs since they failed |
|
I think the R build does need updating from the error |
|
Yes, the postgres driver's bootstrap.R will need to be updated to (1) copy the additional utils.h file and (2) apply the rename of util.h to postgres_util.h (I think!). |
| @@ -206,15 +211,17 @@ struct BindStream { | |||
| param_lengths[i] = 0; | |||
| break; | |||
| default: | |||
| SetError(error, "Field #", i + 1, " ('", bind_schema->children[i]->name, | |||
| SetError(error, "%s%" PRIu64 "%s%s%s%s", "[libpq] Field #", | |||
There was a problem hiding this comment.
The R builds using mingw32 didn't like the %z specifier statement.cc:214:31: warning: unknown conversion type character 'z' in format [-Wformat=]
|
Out of my element with the R stuff but think I got it. Windows build still failing - assuming unrelated to this PR |
