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.

Sign up for GitHub

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

Jump to bottom

Add Default trait to Fe32 (derived) #184

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Add Default trait to Fe32 (derived) #184

merged 1 commit into from
Jul 31, 2024

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Jul 16, 2024 •
edited
Loading

See #187 .

Add derived Default trait to Fe32, and ZERO constant.
Default is trivial to add, but it is useful in some cases.

@optout21 optout21 mentioned this pull request Jul 16, 2024
Closed
apoelstra
apoelstra approved these changes Jul 16, 2024
View reviewed changes
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 c44e602

@apoelstra
Copy link
Member

Can you update the API file? You can run just check-api or read the justfile to see the manual invocation.

@optout21
Copy link
Contributor Author

API doc updated

apoelstra
apoelstra approved these changes Jul 16, 2024
View reviewed changes
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 e700e77

@tcharding
Copy link
Member

Oh there are two PRs (also #185), can you drop the second patch off this PR please.

@tcharding
Copy link
Member

Oh boy, this is all mixed up, I got confused. You have the changes in #184 and #185 in a single patch described in two PRs.

@optout21
Copy link
Contributor Author

I created separate PRs for all the different changes (Default trait, ZERO const, Ord trait), to keep changes minimal.
But the ZERO const was in fact in this PR as well. I've removed it now.

@optout21 optout21 requested a review from apoelstra July 16, 2024 21:19
@clarkmoody
Copy link
Member

Ok, I'd like to see the commit history cleaned up for this one. It can probably be done in a single commit, so squashing might work.

@optout21
Copy link
Contributor Author

Squashed into one commit

tcharding
tcharding approved these changes Jul 17, 2024
View reviewed changes
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 704e891

Thanks man!

apoelstra
apoelstra approved these changes Jul 17, 2024
View reviewed changes
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 704e891

clarkmoody
clarkmoody approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

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

ACK 704e891

@tcharding
Copy link
Member

Looks like this slipped through the cracks, can you merge this please @apoelstra

@tcharding tcharding mentioned this pull request Jul 31, 2024
Draft
@apoelstra apoelstra merged commit 49ddb1c into rust-bitcoin:master Jul 31, 2024
13 checks passed
@optout21 optout21 mentioned this pull request Aug 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding approved these changes

@clarkmoody clarkmoody clarkmoody approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@optout21 @apoelstra @tcharding @clarkmoody