Initial implementation of a StaticShape type #331
Conversation
|
Already a patch for the lints in #330 |
ratmice
force-pushed
the
static_shape
branch
2 times, most recently
from
b9fa1f2
to
e2984b5
Compare
|
This didn't require any changes after #340 but i've rebased it to keep it up to date, and fixed up clippy. |
|
Would it be benifital to make a macro for all the matching? |
|
It didn't seem likely to me that it would be beneficial, because of the |
|
But in this case all the match arms are similar so can't we write a macro for the entire match statement? |
|
This use case is already mostly covered by |
Yeah, looks like that might work, I had previously tried not the entire match statement but the block of match arms,
Mostly, but it does lose the specific primitive |
src/static_shape.rs
Outdated
| @@ -1,6 +1,8 @@ | |||
| // Copyright 2024 the Kurbo Authors | |||
| // SPDX-License-Identifier: Apache-2.0 OR MIT | |||
|
|
|||
| #![allow(unused_qualifications)] | |||
There was a problem hiding this comment.
I didn't see a way to get this lint working, while leaving it enabled
it basically complains that we use crate::$it where $it evaluates to Rect because that is already in scope.
but the macro is called for other types which aren't in scope and for which the qualification isn't unnecessary.
Thanks for explaining. Although I have to wonder if |
|
Indeed, |
| mod _never_shape { | ||
| use super::*; | ||
| use crate::PathEl; | ||
| /// An uninhabited type that implements shape. | ||
| #[derive(Debug, Clone, Copy, PartialEq)] | ||
| #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| pub enum NeverShape {} | ||
| impl Shape for NeverShape { | ||
| type PathElementsIter<'a> = core::iter::Empty<PathEl>; | ||
| fn path_elements(&self, _: f64) -> Self::PathElementsIter<'_> { | ||
| unreachable!() | ||
| } | ||
| fn area(&self) -> f64 { | ||
| unreachable!() | ||
| } | ||
| fn perimeter(&self, _: f64) -> f64 { | ||
| unreachable!() | ||
| } | ||
| fn winding(&self, _: Point) -> i32 { | ||
| unreachable!() | ||
| } | ||
| fn bounding_box(&self) -> Rect { | ||
| unreachable!() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I would recommend removing this block and the whole External shape argument.
The External argument only lets you add one other shape to ConcreteShape anyway. If you want to add multiple "extra" shapes, you need to pass an enum as the External argument, at which point you might as well just implement you own enum instead of ConcreteShape.
There was a problem hiding this comment.
My general feeling is that by using the defaulted type parameter it really doesn't add too much in the way of extra complexity the caller needs to be aware of (unfortunately it's still there to be looked at).
But does preserve the openness of type, meaning it'll becomes possible to use in library code even though someone is using an external shape argument, or across projects which both include extra shapes by making an enum which bridges both.
The need for an extra enum is not desirable, but it seems the only way to make an open enum type.
Without this I fear that someone will want to use the code in library code, and rather than might as well they must implement their own enum instead of using ConcreteShape. Then once people start targeting their own enum as the base of their API it becomes hard to share code across projects that depend upon kurbo.
So I would kind of push back, but I do think it is useful without the extra type parameter, and that custom shape impls seem pretty rare, so I would probably relent if there are strong feelings against this type parameter.
There was a problem hiding this comment.
Fwiw, I also strongly agree with Olivier here. Then enums that these people will make would likely be enum {StaticShape, MyShape}; the scenario where that's an issue is exactly the same scenario where your propsed StaticShape<MyShape> becomes an issue.
(this is apart from the merit of this API at all, on which I have no opinion)
There was a problem hiding this comment.
Sounds like I'm in the minority here then, and I value consensus more than my own opinion, So I'll probably give it some time to see if anyone else chimes in, and otherwise remove it.
By removing it I guess what we lose is the ability to StaticShape<T> where T: Shape from library code, which is kind of what I was aiming for, along with avoiding a proliferation of shape wrappers.
|
FWIW, I think that despite the approval, after some thinking on it I would rather like to investigate the other potential options I outlined in my original PR Message, like the erased_serde style. Which I had not really fully investigated since the "KISS" approach using an enum hit all the qualifications I had outlined in that message... Since after review it no longer hits all those qualifications due to the ugly bits. At least I feel like it is worth revisiting, to see if I can get anything that hits points a) and b) of my original message without the ugly bits that review is removing that cover point b. |
|
So what's the status of this PR? Even if we don't have a consensus on the External parameter (though it kind sounds like we do), I'd still like for this to be merged. |
|
The current status is that after review this patch won't solve all of the points listed in my original PR description, So I would prefer to just hold off until we've fully explored the space, and see if we can't get a type that works with other libraries and shape impl's outside of kurbo, but understand if you just want to have one that works with the types implemented in kurbo itself. because as it is, there isn't a reason libraries/users can't just implement this type itself it is just annoying to duplicate everywhere. |
This brings in the
StaticShapeIt was the best name I could come up with, but I'm not opposed to changing it if we come up with a better one.One of the difficulties with the
Shapetrait as it stands is that:Shapeisn't object safe.erased_serdestyle hacks that can make some non-obj safe traits be obj safe can work with this style of trait that uses GATs.This puts libraries and applications in an awkward pickle, a) there isn't a good way to store them in hetereogenerous collections, and b) because of the open trait there isn't really a single crate that can actually compile list of types implementing
Shape.Given the combined effect of these two things, it feels like a really awkward corner of the language.
This pr attempts to deal with both of these things. For the latter I've added the notion of an
Externalvariant, which can be anyT: Shape, For ergonomics in the case where there is no external variant this defaults to an uninhabited type, so if you are just using kurbo provided shape types, you shouldn't have to worry about that generic.One of the big downsides of this implementation is that it ends up having to use a
Box<dyn Iterator<...>>for thePathElementIterator. I really appreciate everyone who took the time to discuss some of this stuff yesterday on zulip.So thank you & I look forward to your feedback.