Skip to content

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Oct 12, 2022

When working on ADBC, a bit of time was lost because those pointers were used incorrectly. This PR uses pointers to anonymous structs instead of plain void pointers, avoiding the following error kinds:

  • duckdb_database vs. duckdb_connection
  • duckdb_database vs. duckdb_database*

If we agree that this (or a similar approach) is useful, I can proceed with securing the four remaining typedefs which currently break the build.

@hannes hannes requested a review from Mytherin October 17, 2022 11:11
@Mytherin
Copy link
Collaborator

Thanks for the PR! I think this is a good idea, as long as it can be done in a portable manner that does not break any of the builds.

@krlmlr krlmlr force-pushed the f-typed-ptrs branch 3 times, most recently from 93c69d9 to b05ebf5 Compare October 30, 2022 04:59
@krlmlr krlmlr changed the title Use unnamed structs to avoid confusing C pointer wrappers Use structs to avoid confusing C pointer wrappers Oct 30, 2022
@krlmlr
Copy link
Collaborator Author

krlmlr commented Oct 30, 2022

Tests seem much better now, the failure in the extension test seems to be an internet outage.

@Tishj
Copy link
Contributor

Tishj commented Nov 1, 2022

Yea that failure is fixed by a recent PR that changes pulling openssl from github instead of the old source

Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the tests! Looks good to me now, one minor question:

@krlmlr krlmlr requested a review from Mytherin November 6, 2022 19:11
@Mytherin Mytherin changed the base branch from master to feature November 14, 2022 17:25
@Mytherin Mytherin merged commit ee2329b into duckdb:feature Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants