Repository navigation
Cleanup double parsing and formatting code in System.Private.Xml - #54103
huoyaoyuan wants to merge 8 commits into
Conversation
|
Tagging subscribers to this area: @buyaa-n, @krwq Issue DetailsSize of Release build: 3078KB -> 3063KB
|
|
|
||
| Converts a floating point number to a string according to XPath rules. | ||
| */ | ||
| public static string DoubleToString(double dbl) |
There was a problem hiding this comment.
I'll need to see some test cases making sure we return the same as we did before for all edge cases here. I do not feel confident double.ToString(CultureInfo.InvarianCulture) returns the same as this method (not without some kind of test cases reasurrance).
There was a problem hiding this comment.
I haven't looked deeply into the code yet, but the behavior looks to be changed. The old code doesn't seem to output e notations. But I'm also not sure what's the desired behavior.
There was a problem hiding this comment.
That's why I prefer to split this change. We should document the changes in the issue and see what the spec says vs what is happening right now. If current behavior is not matching the spec and new one is then we can change it but let's have the issue first so we can see clearly what's happening and document why the change happens
| // the error; if it does not signal the error, it must recover by converting the number to a string as if | ||
| // by a call to the 'string' function and inserting the resulting string into the result tree. | ||
| return XPathConvert.DoubleToString(dblVal); | ||
| return dblVal.ToString(CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
I'd suggest split this change into removing method by method. I.e. Remove just DoubleToString and add relevant test cases. And do so for all methods you're replacing here. If we regress here we'll need to revert entire changeset here as I doubt anyone will be willing to spend time investigating what went wrong otherwise.
There was a problem hiding this comment.
(for dead code you can just go ahead and remove it though)
There was a problem hiding this comment.
Yeah I'm going to split this.
Size of Release build: 3078KB -> 3063KB