Use generic math to dedup Enumerable.Min/Max by stephentoub · Pull Request #68183 · dotnet/runtime (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.
Issue Details
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.
| Author: | stephentoub |
|---|---|
| Assignees: | stephentoub |
| Labels: | area-System.Linq |
| Milestone: | - |
Comment on lines +65 to 68
| if (span[i] > value) |
|---|
| { |
| value = span[i]; |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Love this. Would it make sense to do this for Math functions too?
--
Note: I filed #64031 and #63732 earlier, now it would just keep track of making these public, if possible.
Would it make sense to do this for Math functions too?
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.
directhex pushed a commit to directhex/runtime that referenced this pull request
Use generic math to dedup Enumerable.Min/Max
Address PR feedback
