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

Implement type conversions on BigNum #1214

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

npmccallum
Copy link
Contributor

@npmccallum npmccallum commented Dec 24, 2019

The type conversions both closely wrap existing (1.1.0) OpenSSL APIs and are idiomatic Rust.

Deprecate: BigNum::to_vec(), BigNum::from_slice() and BigNum::from_u32() in favor of the new idiomatic APIs.

Also, silence warnings about std::mem::uninitialized() on newer Rust.

Minimum Required Rust Version is now 1.34 (for TryFrom).

@npmccallum npmccallum force-pushed the convert branch 4 times, most recently from eb7bb1c to fe792c8 Compare December 25, 2019 03:14
@npmccallum
Copy link
Contributor Author

This PR replaces #1203 and #1204.

@npmccallum
Copy link
Contributor Author

@sfackler ping

@npmccallum
Copy link
Contributor Author

@sfackler bump

@npmccallum
Copy link
Contributor Author

@sfackler We are patching for this downstream. The latest release broke our build. Could you please review this so that we can get it in a release soon? Thanks!

@npmccallum
Copy link
Contributor Author

@sfackler Releasing v0.10.28 broke our build again. Can you please review this PR?

@npmccallum
Copy link
Contributor Author

@sfackler Could you at least comment and let me know the status of this PR? Thanks!

@npmccallum
Copy link
Contributor Author

@sfackler Any update? v0.10.29 broke our build again.

@npmccallum npmccallum force-pushed the convert branch 2 times, most recently from e9e1637 to bb87e29 Compare August 24, 2020 16:29
@npmccallum
Copy link
Contributor Author

@sfackler I've rebased this PR against master.

Today marks 8 months since I first submitted this PR; but I haven't heard anything from you. You still seem to be active in pushing releases, so I'm assuming you're around. I'm happy to trade reviews for something that is a high priority for you (either here or elsewhere...) if you'd like. Please let me know anything I can do to help.

Copy link
Contributor

@petreeftime petreeftime left a comment

Choose a reason for hiding this comment

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

LGTM! Makes conversions a lot more idiomatic, but I think this would need a version bump from 0.10 to 0.11, since it changes the interface quite a bit.

@npmccallum
Copy link
Contributor Author

@petreeftime Although it does change the interface, this patch set is fully backwards compatible. A bump from 0.10 to 0.11 indicates a break in API/ABI which this patch doesn't implement. Do you still want a version bump?

@petreeftime
Copy link
Contributor

I think deprecated functions can break builds for people in some situations, so it might be safer to have a version bump, which would prevent this. I am not an owner of this repo, so ultimately it's not my decision, I just like the patches and wanted to implement something similar.

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.

2 participants