cdaudt commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154951154
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -57,6 +57,11 @@ void GetWinError(std::string* buffer) {
#endif // defined(_WIN32)
+// Struct initializers
+static void AdbcErrorInit(struct AdbcError* error) {
+ std::memset(error, 0, sizeof(*error));
Review Comment:
yes, you're right. So maybe a reason to keep the AdbcErrorInit function, and
add a null check. I missed that comment in adbc.h - if that is the defined
behaviour then the docs should probably replicate it to the individual API
calls (DatabaseNew/ConnectionNew/StatementNew already call out that the
Database/Connection/Statement parameters must be zero initialized).
As a defensive mechanism on the API whoever I do think it should be
explicit in zeroing it, as in the current implementation if it is not zeroed
then you get erratic behaviour (which is how I came across this - I was getting
a segmentation violation because "message" had junk in it going in because I
didn't realize I needed to initialize to zero, and that caused SetError to
incorrectly call error->release() which also contained junk and caused the
segmentation violation).
the need for the "error->release()" call by the API caller is something I
had planned to bring up separately - seems like a bad idea to pass allocation
back in an optional error field that is then responsibility of the caller to
free (and I don't even see that mentioned in the documentation btw).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
