-
Notifications
You must be signed in to change notification settings - Fork 93
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
build(c): Fix linker error with SQLite tests #2260
Conversation
@@ -491,6 +491,14 @@ class StatementTest { | |||
const char* timezone); | |||
}; | |||
|
|||
template <typename CType> | |||
void StatementTest::TestSqlIngestType(ArrowType type, |
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.
There's also TestSqlIngestNumericType
and TestSqlIngestTemporalType
that would be affected by the same issue, although these aren't actually used by any subclasses.
Maybe these should be private methods?
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.
Hmm if not used by subclasses we could keep them in the CC file to hopefully slim down compilation times a tiny bit (though header discipline etc is probably pretty bad anyways)
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.
Hmm if not used by subclasses we could keep them in the CC file
Do you mean to make these free-standing functions in the module? I think the challenge there is that they ultimately wrap TestSqlIngestType
, which relies on some of the class members
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.
I mean that private methods would be fine but ideally we can keep the body in the CC file and not in the header? Or is that not doable
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.
If you are talking about TestSqlIngestType we can move this back to the cc file if we explicitly instantiate the template types that subclasses can use there as well.
The other methods already have their implementations in the cc module
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.
Ah sorry I should actually look into things before I comment...TestSqlIngestNumericType etc are already in the CC file so we can keep them as-is/you can mark them private if you think that helps, but otherwise let's merge this
I think I meant to do the explicit instantiations so that we could keep the body out of the header, though this works too. |
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
@@ -491,6 +491,14 @@ class StatementTest { | |||
const char* timezone); | |||
}; | |||
|
|||
template <typename CType> | |||
void StatementTest::TestSqlIngestType(ArrowType type, |
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.
Ah sorry I should actually look into things before I comment...TestSqlIngestNumericType etc are already in the CC file so we can keep them as-is/you can mark them private if you think that helps, but otherwise let's merge this
Noticed this when compiled via meson:
I'm not sure why the CMake config is OK with this, likely some magic I don't understand. However, it makes sense to me that the linker is complaining; the adbc_validation.h header declares this template but the definition is tucked away in adbc_validation_statement.cc. So I don't think the TU for sqlite_test.cc would have access to that (?) without moving the definition to the header.
Alternatively we could provide an explicit template instantiation for the type (uint64_t) in adbc_validation_statement.cc, though I don't think that is inline with the intent of this class