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

Vector missing the Sum trait #469

Closed
mikepurvis opened this issue Dec 27, 2020 · 3 comments
Closed

Vector missing the Sum trait #469

mikepurvis opened this issue Dec 27, 2020 · 3 comments

Comments

@mikepurvis
Copy link
Contributor

Hey, I'm a new rustacean who did Advent of Code this year in Rust and used the Euclid crate for several of the puzzles. One of the ergonomic quirks I hit was that the Vector types didn't work with sum(), so I ended up having to use a fold instead with a trivial closure:

https://github.com/mikepurvis/advent-of-code/blob/5d9ef10d566dc012f41ccadc5dbcd3474c7c8e19/2020/day-24/src/main.rs#L43-L54

I believe this should be a small change and can submit a PR for it if there's interest. I would apply the change to basically all existing structures which have the AddAssign trait:

https://github.com/servo/euclid/search?q=AddAssign

@nical
Copy link
Contributor

nical commented Dec 27, 2020

Hi, sounds good to me! Euclid has a no-std feature so if you would like to submit a PR, please use core::iter::Sum instead of the std one so things keep working in that configuration.
You would add a Sum implementation to all types that implement AddAssign where the parameter is the same type (for example no Sum for point types since point + point is disallowed.

@mikepurvis
Copy link
Contributor Author

Okay! I got started by implementing it for Angle, and that was fairly trivial, but in order for it to work with iter().sum() as well as iter().copied().sum(), it needed to have a second Sum<&Angle> impl, and this necessitated also adding an Add<&Angle> impl. All this works fine, but before I proceed with the rest of the types, I wanted to check:

  • Is there a reason the other &Angle trait impls don't exist? Should that part be excluded?
  • Is it okay to add them piecemeal as part of this and other changes, or would you like them added as a complete set all at once, either in this change or as a separate one ahead of it?

My work so far is here: master...mikepurvis:sum-traits

@mikepurvis
Copy link
Contributor Author

mikepurvis commented Dec 31, 2020

Closing in favour of #470.

bors-servo added a commit that referenced this issue Jan 4, 2021
Sum traits for Angle, Size, Length, Vector

Addresses #469 with `Add<&Self>`, `Sum<Self>`, and `Sum<&Self>` traits, and filled in a handful of missing tests.

Note that there's a small inconsistency now with operations other than Add not supporting `other = &Self` for these types, but I think if there's concern about that it can be addressed in a follow-on PR.

My only other question on this (as a Rust noob) is just that the sum traits are expressed generically and thus pretty repetitive across all the supported types. I see some instances in the standard library where things like this are [handled across a dozen types at once using a macro](https://doc.rust-lang.org/1.49.0/src/core/iter/traits/accum.rs.html#41). That feels like it may not be worth it for a type or two at a time (eg, handling Vector2D and Vector3D with the same impl), but I was curious if such a thing could be usable for larger blocks of shared functionality, and whether or not that would even be desirable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants