Suggestion: Simplify SettledOrders IntoIterator type#45
Conversation
The previous IntoIterator::IntoIter alias spelled out std::iter::Map<std::iter::Zip<...>, fn(...)> and required a non-capturing closure coerced to a function pointer so the type stayed nameable. Replace it with a tiny SettledOrdersIter struct that holds the two slice iterators and implements Iterator directly. Behavior is unchanged for all call sites.
| fn into_iter(self) -> Self::IntoIter { | ||
| // A non-capturing closure coerced to a function pointer so the iterator | ||
| // type stays nameable in `IntoIter` above. | ||
| let pair_to_order: fn((&'a [AccountView; 2], &'a u8)) -> SettledOrder<'a> = | ||
| |([order_pda, sell_token_account], &bump)| SettledOrder { | ||
| order_pda, | ||
| sell_token_account, | ||
| bump, | ||
| }; | ||
| self.accounts.iter().zip(self.bumps).map(pair_to_order) | ||
| SettledOrdersIter { | ||
| accounts: self.accounts.iter(), | ||
| bumps: self.bumps.iter(), | ||
| } | ||
| } |
There was a problem hiding this comment.
For correctness, I'd include an assert here to confirm both collections hold the same amount of elements.
There was a problem hiding this comment.
im not a rust engineer but something about this change doesn't seem like its actually simplifying as intended. While I understand it unpacks the complicated IntoIter = nicely, we do so by introducing a next() function implementation, we are still left with a longer line count and anyone who doesn't know rust very well is probably confused what is going on/why this is all needed.
additionally, it sounds like this issue may be being resolved/changed b y federico in their more recent change.
I ended up posting a discussion here of other options, including gpiving up on this concept of type construction entirely #36 (comment)
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
I'm fine with this, I also don't like the captured closure anyway. I realize it myself that the current implementation isn't that great by working with it, and as Kaze mentioned I ended up changing it in another PR. We can do it now!
3 participants
The previous
IntoIterator::IntoIteralias spelled outstd::iter::Map<std::iter::Zip<...>, fn(...)>and required a non-capturing closure coerced to a function pointer so the type stayed nameable.Replace it with a tiny
SettledOrdersIterstruct that holds the two slice iterators and implements Iterator directly. Behavior is unchanged for all call sites.