Specialize MapForGrouping::fold
#873
Conversation
`MapForGrouping` is only internally used by `GroupingMapBy` and it only calls `for_each` on it (which in turn rely on `fold`). So I allow the clippy lint `missing_trait_methods` because specialize other methods is simply useless. I could replace `next` body by `unreachable!()` and it would still work fine. Anyway, it will disappear from test coverage now that we have one. I previously wandered how to test and benchmark this. The current tests will test this. And I guess I could benchmark this `fold` specialization through some `into_grouping_map_by` benchmark but I simply don't think it's worth the time: no doubt `adapted_iterator.map.fold` is faster than the default `fold` calling `next` repeatedly. Note that all `GroupingMapBy` methods ultimately rely on this `fold` so this specialization will improve performance for all its methods.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #873 +/- ##
==========================================
- Coverage 94.38% 94.15% -0.24%
==========================================
Files 48 48
Lines 6665 6673 +8
==========================================
- Hits 6291 6283 -8
- Misses 374 390 +16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you.
OT: It seems that MapForGrouping is (essentially) just a special cased Map. IIRC we have some of these special-cased Maps/Folds (e.g. MapOk, FilterOk, FilterMapOk). I wonder if we could - once MSRV reaches 1.75 - replace them by Filter<InnerIterator, impl FnMut(InnerIterator::Item)->V> or the like. Then, they would automatically inherit the specialized methods.
|
After a second thought, I'm pretty sure |
|
Seems cleaner indeed. |
Related to #755
MapForGroupingis only internally used byGroupingMapByand it only callsfor_eachon it (which in turn rely onfold). So I allow the clippy lintmissing_trait_methodsbecause specialize other methods is simply useless. (In case we run clippy with warning it.)I could replace
nextbody byunreachable!()and it would still work fine.Anyway, it will disappear from test coverage now that we have one (see here).
I previously wandered how to test and benchmark this:
foldspecialization through someinto_grouping_map_bybenchmark but I simply don't think it's worth the time: no doubtadapted_iterator.map.foldis faster than the defaultfoldcallingnextrepeatedly. We just don't know how much faster.Note that all
GroupingMapBymethods ultimately rely on thisfoldso this specialization will improve performance for all its methods.