Skip to content

Remove feature gated enum variants#881

Merged
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
tcharding:645-rm-feature-gated-variants
Mar 24, 2022
Merged

Remove feature gated enum variants#881
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
tcharding:645-rm-feature-gated-variants

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Mar 16, 2022

This is the updated version of #874 (which I closed, force pushed, and then was unable to re-open - my bad).

Feature gating enum variants makes code that uses the library brittle while we do not have non_exhaustive, we should avoid doing so. Instead we can add a dummy type that is available when the feature is not turned on. Doing so enables the compiler to enforce that we do not create the error type that is feature gated when the feature is not enabled.

Remove the feature gating around bitcoinconsensus error enum variants.

Closes: #645

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Mar 17, 2022

Needs rebase because of #806, sorry

@tcharding tcharding force-pushed the 645-rm-feature-gated-variants branch from 9bedc84 to f4d015e Compare March 17, 2022 23:44
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 17, 2022

No changes in force push, rebase only.

dr-orlovsky
dr-orlovsky reviewed Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Regarding derives on dumb error types: while we should not underderive, we can overderive. I.e., if the upstream adds derives to the respective error types this will not be a breaking change for them, but it will break our code. Thus, I suggest to do all possible derives for dumb errors, namely Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Debug (Default for errors is a bad practice, I assume).

@tcharding tcharding mentioned this pull request Mar 18, 2022
Open
8 tasks
apoelstra
apoelstra previously approved these changes Mar 21, 2022
View reviewed changes
Copy link
Copy Markdown
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 f4d015e46653a481d41ed06b025e5d090172b1cc

Though I am also a little confused why we're dropping derives in this PR.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 21, 2022

Because of this comment: #874 (comment)

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Mar 21, 2022

Because of this comment: #874 (comment)

I do not agree with that. Commented there

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 21, 2022

Oh, I thought your comment was just in the general case. I'll put some more thought into digesting the whole thread of discussion again. Thanks for re-stating you position.

@tcharding
Remove feature gated enum variants
6ad2902 
Feature gating enum variants makes code that uses the library brittle
while we do not have `non_exhaustive`, we should avoid doing so. Instead
we can add a dummy type that is available when the feature is not turned
on. Doing so enables the compiler to enforce that we do not create the
error type that is feature gated when the feature is not enabled.

Remove the feature gating around `bitcoinconsensus` error enum variants.

Closes: rust-bitcoin#645
@tcharding tcharding force-pushed the 645-rm-feature-gated-variants branch from f4d015e to 6ad2902 Compare March 23, 2022 04:00
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 23, 2022

Rebased on master and added all derives as suggested by @dr-orlovsky (+ Clone).

Though I am also a little confused why we're dropping derives in this PR.

PR no longer does this.

sanket1729
sanket1729 approved these changes Mar 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

tACK 6ad2902. This is an improvment.

But I think the correct solution here is to have a separate error type different from script::Error for the verify and verify_with_flags methods. Maybe this should be dealt with #820

dr-orlovsky
dr-orlovsky approved these changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 6ad2902

@dr-orlovsky dr-orlovsky merged commit d263c0c into rust-bitcoin:master Mar 24, 2022
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Mar 24, 2022

Unlike to others PRs having two ACKs, which I didn't merge since I'd liked to see @apoelstra comments, here his comments were clearly addressed and the PR is trivial, so will not take his time on re-reviewing the latest force-push.

@tcharding tcharding deleted the 645-rm-feature-gated-variants branch March 24, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanket1729 sanket1729 sanket1729 approved these changes

@apoelstra apoelstra apoelstra left review comments

+1 more reviewer

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove feature gate from enum Error

4 participants

@tcharding @apoelstra @dr-orlovsky @sanket1729