Skip to content

Rewrite XslNumber.ConvertToDecimal - #55868

Closed
huoyaoyuan wants to merge 2 commits into
dotnet:mainfrom
huoyaoyuan:xslnumber-convertdecimal
Closed

Rewrite XslNumber.ConvertToDecimal#55868
huoyaoyuan wants to merge 2 commits into
dotnet:mainfrom
huoyaoyuan:xslnumber-convertdecimal

Conversation

@huoyaoyuan

@huoyaoyuan huoyaoyuan commented Jul 17, 2021

Copy link
Copy Markdown
Member

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

@ghost ghost added the area-System.Xml label Jul 17, 2021
@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 buyaa-n and krwq July 24, 2021 05:01
@jeffhandley

jeffhandley commented Jul 24, 2021

Copy link
Copy Markdown
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

krwq commented Aug 12, 2021

Copy link
Copy Markdown
Member

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

@jeffhandley

jeffhandley commented Sep 4, 2021

Copy link
Copy Markdown
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

eiriktsarpalis commented Oct 1, 2021

Copy link
Copy Markdown
Member

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

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

@jkotas jkotas Oct 25, 2021

Copy link
Copy Markdown
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

krwq commented Dec 8, 2021

Copy link
Copy Markdown
Member

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

@huoyaoyuan

huoyaoyuan commented Dec 9, 2021

Copy link
Copy Markdown
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

ghost commented Dec 23, 2021

Copy link
Copy Markdown

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

ghost commented Jan 6, 2022

Copy link
Copy Markdown

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