Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete redundant DivMod implementation #45652

Merged
merged 2 commits into from
Dec 6, 2020
Merged

Delete redundant DivMod implementation #45652

merged 2 commits into from
Dec 6, 2020
+10 −36

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Dec 6, 2020 •
edited
Loading

Related to #45074

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas jkotas requested a review from EgorBo December 6, 2020 14:26
@ghost
Copy link

ghost commented Dec 6, 2020

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #45074

Author: jkotas
Assignees: -
Labels:

area-System.Numerics
Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Dec 6, 2020

I thoughts these are used in some nuget packages for other profiles - the comment confused me. That's why I didn't delete them as part of #45074

am11
am11 reviewed Dec 6, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/FormattingHelpers.cs
}

/// <summary>
/// We don't have access to Math.DivRem, so this is a copy of the implementation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one with similar comment about this helper:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Date.cs

Lines 135 to 138 in ea35f6a

// We don't have access to System.Buffers.Text.FormattingHelpers.DivMod,
// so this is a copy of the implementation.
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static uint DivMod(uint numerator, uint denominator, out uint modulo)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Text.Json is compiled downlevel, so it would need ifdefs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, maybe comment can be updated since there is no System.Buffers.Text.FormattingHelpers.DivMod after this change.

@jkotas jkotas mentioned this pull request Dec 6, 2020
Closed
@jkotas
Copy link
Member Author

jkotas commented Dec 6, 2020

Failures are known issues #44657, #45654, #45660

@jkotas jkotas merged commit a5d0bcf into dotnet:master Dec 6, 2020
@jkotas jkotas deleted the DivMod branch December 6, 2020 20:42
@runfoapp runfoapp bot mentioned this pull request Dec 7, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 left review comments

@EgorBo EgorBo EgorBo approved these changes

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan

@jozkee jozkee Awaiting requested review from jozkee

@layomia layomia Awaiting requested review from layomia

@steveharter steveharter Awaiting requested review from steveharter

Assignees
No one assigned
Labels
area-System.Numerics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkotas @Dotnet-GitSync-Bot @EgorBo @am11