Skip to content

Fix #49660 - Adds checks to ensure existence of arithmetic trait implementations - #78259

Merged
bors merged 3 commits into
rust-lang:masterfrom
plaflamme:fix-49660
Jan 14, 2021
Merged

Fix #49660 - Adds checks to ensure existence of arithmetic trait implementations#78259
bors merged 3 commits into
rust-lang:masterfrom
plaflamme:fix-49660

Conversation

@plaflamme

@plaflammeplaflamme commented Oct 23, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

The first 2 commits fix an issue with the existing wrapping.rs tests. It wasn't referred to from the module, so the file was being ignored. This is fixed in 872dc60 This surfaced a bug in its macro which is fixed in 8ddad18

Lastly, commit 64d695b is the actual tests for fixing #49660

The following checks are done:

  • Add, Sub, Mul, Div, Rem
    • T op T, T op &T, &T op T and &T op &T
    • for all integer and floating point types
  • AddAssign, SubAssign, MulAssign, DivAssign, RemAssign
    • &mut T op T and &mut T op &T
    • for all integer and floating point types
  • Neg
    • op T and op &T
    • for all signed integer and floating point types
  • Not
    • op T and op &T
    • for bool
  • BitAnd, BitOr, BitXor
    • T op T, T op &T, &T op T and &T op &T
    • for all integer types and bool
  • BitAndAssign, BitOrAssign, BitXorAssign
    • &mut T op T and &mut T op &T
    • for all integer types and bool
  • Shl, Shr
    • L op R, L op &R, &L op R and &L op &R
    • for all pairs of integer types
  • ShlAssign, ShrAssign
    • &mut L op R, &mut L op &R
    • for all pairs of integer types

NOTE: I'd like some feedback on improving the macros. I'm not familiar with the idioms and patterns there and composing them has been a challenge for me.

[EDIT]: updated links to commits after rebase.

@rust-highfive

rust-highfive commented Oct 23, 2020

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2020
@Dylan-DPC-zz

Dylan-DPC-zz commented Nov 10, 2020

Copy link
Copy Markdown

r? @KodrAus

@crlf0710crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2020
@crlf0710crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api [DEPRECATED; DO NOT USE] and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2020
@JohnCSimon

JohnCSimon commented Jan 11, 2021

Copy link
Copy Markdown

Ping from triage
@plaflamme can you please resolve the merge conflict and alert the reviewer? Thank you!

@KodrAus

KodrAus commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

This looks like a great safety net to me! r=me after a rebase

@plaflamme

plaflamme commented Jan 14, 2021

Copy link
Copy Markdown
ContributorAuthor

@KodrAus I was actually working on this just now... I've merged and fixed the conflicts. Please take a look

@KodrAus

KodrAus commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

@plaflamme Looks good to me! Would you like to squash these commits down (we try to avoid any merge commits if we can) then it should be good to go.

r=me after that.

@plaflamme
Fix missing mod declaration for Wrapping tests.
872dc60
@plaflamme
Avoid ident concatenation in macro.
8ddad18
AFAIK it isn't currently possible to do this. It is also more in line with other tests in the surrounding modules.
@plaflamme
Adds tests to ensure some base op traits exist.
64d695b
These tests invoke the various op traits using all accepted types they
are implemented for as well as for references to those types.
This fixesrust-lang#49660 and ensures the following implementations exist:
* `Add`, `Sub`, `Mul`, `Div`, `Rem`
* `T op T`, `T op &T`, `&T op T` and `&T op &T`
* for all integer and floating point types
* `AddAssign`, `SubAssign`, `MulAssign`, `DivAssign`, `RemAssign`
* `&mut T op T` and `&mut T op &T`
* for all integer and floating point types
* `Neg`
* `op T` and `op &T`
* for all signed integer and floating point types
* `Not`
* `op T` and `op &T`
* for `bool`
* `BitAnd`, `BitOr`, `BitXor`
* `T op T`, `T op &T`, `&T op T` and `&T op &T`
* for all integer types and bool
* `BitAndAssign`, `BitOrAssign`, `BitXorAssign`
* `&mut T op T` and `&mut T op &T`
* for all integer types and bool
* `Shl`, `Shr`
* `L op R`, `L op &R`, `&L op R` and `&L op &R`
* for all pairs of integer types
* `ShlAssign`, `ShrAssign`
* `&mut L op R`, `&mut L op &R`
* for all pairs of integer types
@plaflamme

plaflamme commented Jan 14, 2021

Copy link
Copy Markdown
ContributorAuthor

@KodrAus I've rebased and force pushed, removing the merge commit. Let me know if you'd like me to squash all of them. I kept the ones affecting wrapping.rs separate since the changes are unrelated to the original issue.

@KodrAus

KodrAus commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

Thanks @plaflamme!

@bors r+

@bors

bors commented Jan 14, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 64d695b has been approved by KodrAus

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2021
@bors

bors commented Jan 14, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 64d695b with merge a4f022e...

@bors

bors commented Jan 14, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: KodrAus
Pushing a4f022e to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jan 14, 2021
@bors
bors merged commit a4f022e into rust-lang:masterJan 14, 2021
@rustbotrustbot added this to the 1.51.0 milestone Jan 14, 2021
@borsbors mentioned this pull request Jan 14, 2021
Merged
@borsbors mentioned this pull request Jan 14, 2021
Merged
@plaflamme
plaflamme deleted the fix-49660 branch January 14, 2021 15:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@KodrAusKodrAus

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Milestone

1.51.0

Development

Successfully merging this pull request may close these issues.

9 participants

@plaflamme@rust-highfive@Dylan-DPC-zz@JohnCSimon@KodrAus@bors@crlf0710@sfackler@rustbot