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

Rewrite XslNumber.ConvertToDecimal #55868

Closed
wants to merge 2 commits into from
Closed

Rewrite XslNumber.ConvertToDecimal #55868

wants to merge 2 commits into from

Conversation

huoyaoyuan
Copy link
Member

Split from #54103
This is to remove a dependency to XPathConvert.DoubleToString. I don't know if there are test for this.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley jeffhandley requested review from krwq and buyaa-n July 24, 2021 05:01
@jeffhandley
Copy link
Member

@krwq This PR is assigned to you for follow-up/decision before the RC1 snap.

@krwq krwq added this to the 7.0.0 milestone Aug 12, 2021
@krwq krwq added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 12, 2021
@krwq
Copy link
Member

krwq commented Aug 12, 2021

We should consider merging this after RC1 snap. Marking as no merge for now

@jeffhandley
Copy link
Member

We should consider merging this after RC1 snap. Marking as no merge for now

@krwq Can you take a look at this again now that main targets .NET 7.0? Thanks!

@runfoapp runfoapp bot mentioned this pull request Sep 24, 2021
Closed
@eiriktsarpalis eiriktsarpalis removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 1, 2021
@eiriktsarpalis
Copy link
Member

Bump. I think we can take another look at this now.

jkotas
jkotas reviewed Oct 25, 2021
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XslNumber.cs
{
NumberFormatInfo info = nativeDigits == null
? NumberFormatInfo.InvariantInfo
: new NumberFormatInfo
Copy link
Member

Choose a reason for hiding this comment

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

How does the performance of this rewrite compares to the existing implementation?

The existing implementation does not allocate NumberFormatInfo. Is allocating of NumberFormatInfo going to regress performance?

@krwq krwq added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 8, 2021
@krwq
Copy link
Member

krwq commented Dec 8, 2021

We need some perf measurements here. Current version because of all of the NumberFormatInfo related allocations might be slower than original

@huoyaoyuan
Copy link
Member Author

Sorry, I have no time to do the measurement recently.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 9, 2021
@krwq krwq added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 9, 2021
@ghost ghost added the no-recent-activity label Dec 23, 2021
@ghost
Copy link

ghost commented Dec 23, 2021

This pull request has been automatically marked no recent activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

@ghost
Copy link

ghost commented Jan 6, 2022

This pull request will now be closed since it had been marked no recent activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this Jan 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2022
@huoyaoyuan huoyaoyuan deleted the xslnumber-convertdecimal branch May 16, 2023 19:04
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@krwq krwq Awaiting requested review from krwq

@buyaa-n buyaa-n Awaiting requested review from buyaa-n

Assignees

@krwq krwq

Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@huoyaoyuan @jeffhandley @krwq @eiriktsarpalis @jkotas @terrajobst