search

paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
290 stars 218 forks source link

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

Closed halo3mic closed 2 years ago

halo3mic commented 2 years ago

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

Fixes #401

halo3mic commented 2 years ago

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

Sure, are the ones I just added okay?

AaronKutch commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 :)