blippy: introduce BlippyOrd<T> to order notes containing non-Ord types - #11290
jgallagher wants to merge 1 commit into
Conversation
bnaecker
left a comment
bnaecker
left a comment
There was a problem hiding this comment.
I like this approach too. I think I have a weak preference for this over #11284, which it uses the "normal" Ord machinery. But using a dedicated method as in that PR has benefits, mostly around explicitness and being harder to misuse. Either seems strictly better than what we have!
| IdOrdMap<OmicronZoneExternalFloatingIp>, | ||
| ); | ||
|
|
||
| impl std::cmp::PartialOrd for OmicronZoneExternalFloatingIps { |
| DuplicateUnderlayIp { | ||
| zone1: BlueprintZoneConfig, | ||
| zone2: BlueprintZoneConfig, | ||
| zone1: BlippyOrd<BlueprintZoneConfig>, |
There was a problem hiding this comment.
Does it matter that this will show up in the Debug output? As in, do we want to "delegate" that to the implementation on the inner type? I don't know how often we look at that representation, maybe there are special "reporting" methods we lean on instead.
There was a problem hiding this comment.
Mm yeah good call. If we go with this route I'll remove the #[derive(Debug)], and manually implement it as a pass through to the inner type's Debug.
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
This is okay, but I don't like that this approach permeates all the places the type gets used instead of being localized to the bits that need the ordering.
| impl<T: Clone> From<&'_ T> for BlippyOrd<T> { | ||
| fn from(value: &'_ T) -> Self { | ||
| Self(value.clone()) | ||
| } | ||
| } |
There was a problem hiding this comment.
It makes some of the conversions shorter 🤷♂️. Blippy works almost entirely in terms of references, except when it needs to construct a note, at which point it clones. I could remove this and some foo.into()s would turn into foo.clone().into()?
There was a problem hiding this comment.
I was more wondering: why can't the BlippyOrd contain the reference in that case?
There was a problem hiding this comment.
It might be able to. I suspect that ends up causing ownership issues somewhere or we would've used references for all the types contained by Kind already, but I could try it and see.
|
I wonder if there's something in between, where we create a separate type that impls |
I don't know how we'd do that without the concrete type that we return being a duplicate of the entire |
3 participants
This is an alternative to #11284. I'm not at all sure it's better; I don't really love either of them. I wanted to try "stick a non-
Ordtype in anOrdwrapper" and since I have this compiling, figured I could open this and we could compare.