search

servo / euclid

Geometry primitives (basic linear algebra) for Rust
Other
462 stars 102 forks source link

Sum traits for Angle, Size, Length, Vector #470

Closed mikepurvis closed 3 years ago

mikepurvis commented 3 years ago

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. 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.

nical commented 3 years ago

Thanks for this PR. I'm not in favor of implementing Add and similar traits for &T It's simple enough to dereference the types and sum can be implemented on top of Add<Self>.

mikepurvis commented 3 years ago

@nical Thanks for the feedback. My logic for doing it this way was just to avoid needing a custom closure (to supply the dereference) for the Sum<&Self> case, and also because this is what std implements for the basic types, eg:

https://doc.rust-lang.org/1.49.0/src/core/iter/traits/accum.rs.html#57-62

Just to ensure that I understand your request, can you confirm that you'd like the Add<&Self> implementations removed, but both Sum<Self> and Sum<&Self> can remain?

nical commented 3 years ago

Huh, TIL that the standard library implements f32 + &f32 and the likes. Alright then. Thanks for pointing this out.

@bors-servo r+

bors-servo commented 3 years ago

:pushpin: Commit 8ef764f has been approved by nical

bors-servo commented 3 years ago

:hourglass: Testing commit 8ef764f3dc7571ea800b140cbfa26c198265907c with merge a9fa27f5d63c81633e20adc2173c2da2232a58cc...

bors-servo commented 3 years ago

:sunny: Test successful - checks-travis Approved by: nical Pushing a9fa27f5d63c81633e20adc2173c2da2232a58cc to master...

mikepurvis commented 3 years ago

Great, thanks for the merge!