Use generic math to dedup Enumerable.Min/Max #68183
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-linq Issue DetailsFor each method where the same code was duplicated for multiple types, I just made one of the implementations generic and private and delegated to it from each of the public signatures. I did not change any logic, though I did tweak a few of the existing comments.
|
| if (span[i] > value) | ||
| { | ||
| value = span[i]; | ||
| } |
There was a problem hiding this comment.
I've done various checks for code I've looked at locally, but it would be nice to validate there aren't any codegen changes for the scalar fallback path here.
Would help indicate if there is a need to tweak the inliner at all for these scenarios.
There was a problem hiding this comment.
CC. @EgorBo. Might be interesting to give generic math operators an inline multiplier, particularly if small or for the primitive types (which are normally single inline instructions)
There was a problem hiding this comment.
it would be nice to validate there aren't any codegen changes for the scalar fallback path here.
This is Max on IEnumerable<int?>... it looks ok to me:
https://www.diffchecker.com/MU3Kg6ow
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
Looks good/correct to me.
Left a callout that I think we could actually get this down to a single implementation, but it would require more in depth changes to the interface API surface
Which ones in particular? Most of the Math functions that operate over many different types are very short and providing a shared generic implementation wouldn't save much. |
* Use generic math to dedup Enumerable.Min/Max * Address PR feedback
ghost
locked as resolved and limited conversation to collaborators
3 participants
For each method where the same code was duplicated for multiple types, I just made one of the implementations generic and private and delegated to it from each of the public signatures. I did not change any logic, though I did tweak a few of the existing comments.