servo / euclid

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

(breaking change) Make intersection return Option<Self> #463

Closed nical closed 4 years ago

nical commented 4 years ago

Replaces #462

As I go through the webrender update I realize that returning Option<NonEmpty<Self>> adds way more friction than I anticipated. Pretty much all call sites have to be worked around in different ways. #462 helps for at least a good half of the call sites, but there are many other patterns, and I am not running into cases where NonEmpty is adding value, because we end up having to deal with storing something when the intersection is empty (the way webrender's code is shaped).

I'm very woriied that intersection is just going to be a pain to use by default, with NonEmpty providing more friction than value, so considering 0.21 is just out, I would rather make a early 0.22 now before a significant part of downstream users have had time to update to 0.21, and get rid of the thorn before we are stuck with it.

Note that for cases where Option<NonEmpty<Box2D>> would have been desirable, we can still write `b.intersection_unchecked(b2).to_non_empty(), which is not too inconvenient even if not the default.

kvark commented 4 years ago

I'd love to get a glimpse of some of the friction you faced

kvark commented 4 years ago

What is the value in having NonEmpty in the type system if it's not used now?

nical commented 4 years ago

I don't mind removing NonEmpty, we don't use it at all in webrender or servo, and we can reevaluate adding it later after spending time figuring out how to do it well.

nical commented 4 years ago

@bors-servo r+

bors-servo commented 4 years ago

:pushpin: Commit 32c352a has been approved by nical

bors-servo commented 4 years ago

:hourglass: Testing commit 32c352a09225e98952f333759d2c157a207832e0 with merge 289407554efb7a9fb82030529e01fa0b914b4813...

bors-servo commented 4 years ago

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