Skip to content
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

regenerate with recent bindgen #11

Merged
merged 2 commits into from
Sep 11, 2017
Merged

regenerate with recent bindgen #11

merged 2 commits into from
Sep 11, 2017

Conversation

dimbleby
Copy link
Contributor

I was messing around with updating bindgen on my own project today, and though I'd see what it currently did to these cassandra bindings.

For the record, what I did was:

$ bindgen --no-layout-tests --blacklist-type=max_align_t --output=src/cassandra.rs cassandra.h

followed by running the output through rustfmt.

So here are the results. Take 'em or leave 'em, I don't really mind!

This seems to be sufficiently back-compatible that cassandra-rs still compiles without complaint, but I expect that some of the changes in here are somehow technically breaking and so publishing would want a version bump.

Main differences, as it seems to me:

  • correctly spots that cass_int8_t should always be signed
    • NB with the version 2.4.3 cassandra.h that you have this is ambiguous depending on build environment because cass_int8_t is sometimes mapped to char, whose signedness is undefined
    • but that's fixed upstream
  • derives more stuff (eg making redundant some of ffi_util.rs)
    • (Except that Clone is manually implemented, because reasons)
  • doesn't [#link(name = "cassandra")] any more
    • (even if you tell it to do so)
    • for which I have compensated in build.rs
  • preserves comments
  • generates consts for some #defines
  • implements opaque types slightly differently

@dimbleby
Copy link
Contributor Author

I somehow forgot about the change that actually is breaking, albeit trivially:

  • size_t is now recognised as usize, rather than being its own special type

@kw217
Copy link
Collaborator

kw217 commented Sep 11, 2017

Thanks, that's interesting. I guess the key question is - what are the advantages of doing this? It's working at the moment so I'm hesitant to change it without good reason.

@dimbleby
Copy link
Contributor Author

Yeah, the differences are pretty small, which cuts both ways: it's not that risky, but it's also not that valuable.

The signed char thing is I think unambiguously a bug fix and the others mostly look like improvements, albeit rather small ones.

Maybe the main win from this pull request is the knowledge that recent bindgen is still working fine for us - we're not storing up technical debt for when we do want to regenerate eg to upgrade the underlying C library. Of course you get that advantage without actually having to merge ...

@kw217
Copy link
Collaborator

kw217 commented Sep 11, 2017

Well, it's encouraging that cassandra-cpp still builds OK (does it pass its tests as well?). It shouldn't need a version bump, because no cassandra-cpp-sys types should be exposed by cassandra-cpp - they should all be wrapped with cassandra-cpp types. It would be nice to have the comments, and to know that we're more up to date.

Please can you add your "for the record" instructions to the docs somewhere, and add that to this PR? Then (as long as cassandra-cpp does indeed build and pass its tests) I am minded to merge. Thanks!

@dimbleby
Copy link
Contributor Author

I agree that there should be no need to bump the version number for cassandra-cpp - unless there's a policy of keeping it in sync with this -sys crate, whose version number definitely does want bumping.

There's one trivial example of the size_t -> usize thing to fix in cassandra-cpp when taking this.

I seem to be not well placed to run the cassandra-cpp tests - something does go wrong for me but it feels environmental rather than fundamental. (I'm seeing errors like "Keyspace system_schema does not exist")

I'll add a sentence to the README saying how the autogeneration was done.

Signed-off-by: David Hotham <[email protected]>
@dimbleby
Copy link
Contributor Author

(I had another go at the cassandra-cpp tests and everything passed this time... not sure what changed!).

@bossmc
Copy link

bossmc commented Sep 11, 2017

It's a reward for writing good docs 😀

Copy link
Collaborator

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

LGTM!

@kw217
Copy link
Collaborator

kw217 commented Sep 11, 2017

Thanks! Merged, released, and now incorporating in cassandra-cpp in cassandra-rs/cassandra-rs#29

@dimbleby dimbleby deleted the recent-bindgen branch September 11, 2017 15:24
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.

3 participants