servo / euclid

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

(breaking change) self vs &self and non-copy scalars #454

Closed nical closed 4 years ago

nical commented 4 years ago

Two sorta dependent questions, hopefully the last bikeshed for this wave of breaking changes.

self vs &self

As a rule of thumb euclid passes large-ish structs by ref (around 4 scalars and more) and smaller things by value.

While the rule is followed mostly consistently for regular arguments, self arguments are a bit of a mix and match.

For example:

We could:

Non-Copy scalars

An extension to this question is: should we get out of our way to support non-Copy scalars? Currently they kind of work for a portion of the methods (where calling .clone() everywhere isn't too annoying).

Personally I would like to just require Copy everywhere. This would mean the question of &self vs self would not impact the user, we'd have no support for big-nums and the likes instead of an insufficient partial support that doesn't make euclid a good fit for that use case anyway, and simplify the code and docs.

Requiring Copy everywhere can be backward compatibly relaxed so in doubt we could do that and change our minds later if/when a real-words use case comes up.

nical commented 4 years ago

cc @kvark

kvark commented 4 years ago

This would mean the question of &self vs self would not impact the user

If we consider the scope of the issue to be larger than self, i.e. when do we pass by value versus by reference, than this will still impact the users.

nical commented 4 years ago

Sorry my phrasing was a bit clumsy. I do think we should keep the current rule about what we pass by ref and by value for non-self parameters. I believe it works well and changing it would require updating so much code that we'd want a really strong motivation. So I'm referring only to the self parameter.

What I meant was that for non-copyable types it matters a great deal whether or not the methods consumes self, whereas if every vector is Copy no matter what, &self vs self is still a breaking change but not something that would limit or increase the expressiveness of the API or make much of a difference in how the API is used (I think).

kvark commented 4 years ago

I'd prefer the API to just be consistent with regards to references. We shouldn't make an exception for self. Otherwise, what would your guideline be? "if it's less than 16 bytes, pass by value, unless it's self parameter, and type doesn't implement Copy, and a lot of user code doesn't want to clone"? Can we just say that all the types that our guideline requires to be passed by value would implement Copy. That would mean we are never in a situation where the user needs to do something.clone().foo(...).

nical commented 4 years ago

Looks like we are on the same page. Just wanted to enumerate options and test the water before writing all the code.