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

After MSRV bump: Implemented TryFrom<{u8, i32}> for Parity #409

Merged
merged 2 commits into from Jun 15, 2022

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Feb 24, 2022

No description provided.

@Kixunil Kixunil force-pushed the parity-impl-try-from branch 2 times, most recently from 670da82 to e148753 Compare February 28, 2022 20:15
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 28, 2022

Added commit changing cause() to source()

@Kixunil Kixunil marked this pull request as ready for review June 10, 2022 11:29
This implements `source()` method instead of `cause()` allowing
downcasting.
@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 10, 2022

Rebased, @tcharding my previous PR had cause->source change so I left it in, do you want me to remove it in favor of your PR?

@tcharding
Copy link
Member

tcharding commented Jun 13, 2022

You were first, leave yours as it is, I'll rebase mine on top of yours. I've removed the std::error::Error stuff from mine and re-worked it to include the write_err macro.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK df081be

@sanket1729
Copy link
Member

What was the reason that we could not have Parity::Even and Parity::Odd. Why do we have an i32 wrapping?

If the underlying FFI guarantees that the value is either 1 or 0. We should represent it in an enum. IMO, there is no need to stick to i32 just because that's the return type of C code.

@tcharding
Copy link
Member

Did you get mixed up? We do have an enum


/// Represents the parity passed between FFI function calls.
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub enum Parity {
    /// Even parity.
    Even = 0,
    /// Odd parity.
    Odd = 1,
}

:)

apoelstra added a commit that referenced this pull request Jun 15, 2022
946cd83 Improve Error display (Tobin C. Harding)

Pull request description:

  Introduce `write_err` macro and improve the `Display` implementation on `Error`.

  This PR is re-written after review below. The `std::error::Error` implementation is removed in favour of #409. Now only includes the `Display` stuff.

ACKs for top commit:
  apoelstra:
    ACK 946cd83 nice!

Tree-SHA512: a62e9593c2ed1ba6136136e6b575219d68c25736069f55448f8411048efae27f2467bcb65260a78ff065de9c8c2994873804c687fb37a55b705cddfe20544f37
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK df081be

@apoelstra
Copy link
Member

@sanket1729 for the purpose of encoding it in a taproot control block, which requires an integer (a u1 not an i32, but ok)

@apoelstra apoelstra merged commit 73ad30d into rust-bitcoin:master Jun 15, 2022
@Kixunil Kixunil deleted the parity-impl-try-from branch June 15, 2022 15:20
@sanket1729
Copy link
Member

Sorry, my local tree was not updated. And I was looking at some old code :(.

We might not need the TryFrom from i32, but I have no complaints about having it either way.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 16, 2022

@sanket1729 FYI my motivation for adding them is based on the Rust API guidelines (and I agree with that particular recommendation :))

@apoelstra
Copy link
Member

@Kixunil for sure -- if we support conversion to/from numerics then we should definitely use TryFrom to do it. But I understood Sanket's question as being "why do we support these conversions at all?".

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.

None yet

4 participants