Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Remove Copy from Ensure* traits#13043

Merged
paritytech-processbot[bot] merged 8 commits intomasterfrom
oty-ensure-without-copy
Jan 4, 2023
Merged

Remove Copy from Ensure* traits#13043
paritytech-processbot[bot] merged 8 commits intomasterfrom
oty-ensure-without-copy

Conversation

@ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 2, 2023 •
edited
Loading

Follow up on #12967 (comment) to make BaseArithmetic work with Ensure.
It re-uses the code from EnsureOpAssign in EnsureOp, instead of the other way around, which incurred the Copy requirement.
It does contain an artificial dependence of EnsureOp on EnsureOpAssign to re-use code, but that should be fine since the trait bounds would be the same anyway.

Removes the Copy requirement from:

  • EnsureOp and EnsureOpAssign
  • EnsureFrom and EnsureInto: Not sure about this one since we now generate the error in every case. Could be slightly slower.
  • Require Ensure for BaseArithmetic

cc @lemunozm

ggwpez added 3 commits January 2, 2023 13:07
@ggwpez
Remove Copy from EnsureOp and EnsureOpAssign
8ffc145 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Remove Copy from EnsureFrom and EnsureInto
597e61d 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix default impl
0bab4c5 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 2, 2023
@ggwpez ggwpez mentioned this pull request Jan 2, 2023
Merged
@ggwpez ggwpez marked this pull request as draft January 2, 2023 12:18
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 2, 2023
@ggwpez
Reuse assignment code in Ensure trait
95b1674 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review January 2, 2023 12:27
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 2, 2023
@ggwpez
Require Ensure for all BaseArithmetic types
8d3b649 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
lemunozm
lemunozm reviewed Jan 2, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

I like this without the Copy dependency 😄 . Some comments below

primitives/arithmetic/src/traits.rs
/// ```
fn ensure_from(other: T) -> Result<Self, ArithmeticError> {
Self::try_from(other).map_err(|_| error::equivalent(other))
let err = error::equivalent(&other);
Copy link
Contributor

@lemunozm lemunozm Jan 2, 2023

Choose a reason for hiding this comment

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

Regarding the performance of this, it seems like the assembly resulting code of this can be easily reordered by the compiler and should not be penalty

Copy link
Member

@bkchr bkchr Jan 2, 2023

Choose a reason for hiding this comment

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

Did you check this?

Copy link
Contributor

@lemunozm lemunozm Jan 3, 2023

Choose a reason for hiding this comment

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

Not empirically (not sure how to do it well).

To extend my argument, error::equivalent has no side effects, and if the resulting value is only read under a condition, the compiler can only execute that for that condition. I understand that this works for any T that is not really consumed by the try_from(), which means that T is in fact a Copy type. The compiler could not optimize this if T is not Copy.

Copy link
Member

@bkchr bkchr Jan 3, 2023

Choose a reason for hiding this comment

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

Okay ;)

ggwpez added 3 commits January 2, 2023 16:06
@ggwpez
Fix assign impls
35c3cc8 
Co-authored-by: Luis Enrique Muñoz Martín <lemunozm@gmail.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Add tests
6c1e4d9 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Add success doc tests
78e8611 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
lemunozm
lemunozm approved these changes Jan 2, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM!
Nice tests 🚀

@ggwpez ggwpez requested a review from bkchr January 2, 2023 15:35
@bkchr bkchr requested a review from kianenigma January 2, 2023 23:05
@ggwpez
Copy link
Member Author

ggwpez commented Jan 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 68fc275 into master Jan 4, 2023
@paritytech-processbot paritytech-processbot bot deleted the oty-ensure-without-copy branch January 4, 2023 13:00
sam0x17
sam0x17 reviewed Jan 4, 2023
View reviewed changes
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@librelois librelois mentioned this pull request Jan 23, 2023
Closed
@librelois librelois mentioned this pull request Jan 23, 2023
Closed
@aurexav aurexav mentioned this pull request Feb 8, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Feb 14, 2023
Merged
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
@ggwpez @ltfschoen
Remove Copy from Ensure* traits (paritytech#13043)
d4c5c55 
* Remove Copy from EnsureOp and EnsureOpAssign

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove Copy from EnsureFrom and EnsureInto

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix default impl

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Reuse assignment code in Ensure trait

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Require Ensure for all BaseArithmetic types

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix assign impls

Co-authored-by: Luis Enrique Muñoz Martín <lemunozm@gmail.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add success doc tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@ggwpez @ark0f
Remove Copy from Ensure* traits (paritytech#13043)
787bb1f 
* Remove Copy from EnsureOp and EnsureOpAssign

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove Copy from EnsureFrom and EnsureInto

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix default impl

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Reuse assignment code in Ensure trait

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Require Ensure for all BaseArithmetic types

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix assign impls

Co-authored-by: Luis Enrique Muñoz Martín <lemunozm@gmail.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add success doc tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kianenigma kianenigma Awaiting requested review from kianenigma

4 more reviewers

@sam0x17 sam0x17 sam0x17 left review comments

@bkchr bkchr bkchr approved these changes

@lemunozm lemunozm lemunozm approved these changes

@ruseinov ruseinov ruseinov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ggwpez @ruseinov @sam0x17 @bkchr @lemunozm