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

Uint: Add support for bit and,or,xor assign traits #690

Merged
merged 3 commits into from Nov 7, 2022

Conversation

halo3mic
Copy link
Contributor

@halo3mic halo3mic commented Nov 6, 2022

Add support uint support for traits BitAndAssign, BitXorAssign and BitOrAssign .

Fixes #401

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks! Could you add some tests for the added methods, please?

@halo3mic
Copy link
Contributor Author

halo3mic commented Nov 6, 2022

Thanks! Could you add some tests for the added methods, please?

Sure, are the ones I just added okay?

@ordian ordian added A8-looksgood changelog Needs to be added to the changelog labels Nov 6, 2022
x ^= b;
assert_eq!(x, a ^ b);
}
// shr
Copy link
Member

Choose a reason for hiding this comment

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

It looks like shr and shl were already implemented and you just added tests for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct, I couldn't find the tests for them and figured it wouldn't hurt to add them.

@ordian ordian merged commit 82b2147 into paritytech:master Nov 7, 2022
@AaronKutch
Copy link

AaronKutch commented Jan 17, 2023

Just FYI you broke semver and one of my crates with this (edit: not this PR in and of itself, the 0.9.5 update should have been 0.10)

@ordian
Copy link
Member

ordian commented Jan 17, 2023

@AaronKutch could you share what broke in 0.9.5 compared to 0.9.4? This is the only change there.

Or did you mean that some version in the 0.9.x series was semver breaking? In that case I would suspect 0.9.2 https://github.com/paritytech/parity-common/blob/master/uint/CHANGELOG.md#092---2022-01-28 breaking MSRV, which we I personally consider non-breaking, but some people disagree.

@AaronKutch
Copy link

I had implemented some traits for the struct from uint::construct_uint! that the macro did not implement before, and after some 0.9 patch there was a trait collision. I guess I am breaking MSRV sometimes myself, but adding traits is more likely to cause problems in the circumstance of structs from macros that can circumvent the orphan rule.

@ordian
Copy link
Member

ordian commented Jan 17, 2023

I see. Thanks for the clarification.

If we follow that logic, then adding an inherent method would also be considered breaking, because a user could have added a method with the same name as well.

The docs says adding adding an inherent item is considered a "possibly-breaking" change (not for the macro case though): https://doc.rust-lang.org/cargo/reference/semver.html#possibly-breaking-change-adding-any-inherent-items.

I'll consider major release for such changes next time. Thanks for bringing this to my attention :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-looksgood changelog Needs to be added to the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help with primitive-types
4 participants