Fix invalid "double.ToString" behavior with two section separators

[Maximys]

Fixes #70460

[ghost]

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

---

null

Author:	Maximys
Assignees:	-
Labels:	area-System.Numerics
Milestone:	-

[tannergooding]

Can you link to the corresponding bug that describes the behavior and what you're fixing?

[Maximys]

Can you link to the corresponding bug that describes the behavior and what you're fixing?

Yes, of course: #70460
Current PR, current fixes is invalid (a lot of test was broken), I'll try to fix it later.

[danmoseley]

@Maximys If you put "Fixes #70460" on its own line in the top comment, the issue will be closed on merge.

[Maximys]

@danmoseley , thank you.

[Maximys]

I think, my fix is ready. How I can change PR from "draft" to normal?

[danmoseley]

click this

[tannergooding]

I think this needs to print -0.00 given that -0.001 rounds to -0.0d and not +0.0d

[Maximys]

Fixed by dab3fe800c27d81f217e09288824fbb9f2afc274 commit

[Maximys]

@tannergooding , MSDN have next info about ";" section separator:

The first section applies to positive values and zeros, and the second section applies to negative values.

If the number to be formatted is negative, but becomes zero after rounding according to the format in the second section, the resulting zero is formatted according to the first section.

If I correctly understand this info, then output "+0.00" is absolutly valid and my commit 6c3d9e6 with fixes by your comment was invalid.
What do you think?

[tannergooding]

The section was written without the consideration around -0.0 and its relevance to floating-point code. We only made the push towards ensuring things were IEEE 754 compliant starting around .NET Core 2.1

The general intent of -0.0 is to represent a very small negative number that was rounded towards zero, but which may not itself be zero. Preserving this semantic and treating it as negative is generally desirable and consistent with the handling we expose elsewhere.

We should likely update the docs to cover that:

• One section - The format string applies to all values.
• Two sections - The first section applies to positive values and zeros, and the second section applies to negative values including negative zero for IEEE 754 floating-point types
• Three sections - The first section applies to positive values, the second section applies to negative values, and the third section applies to zero including negative zero for IEEE 754 floating-point types

That still preserves the ability to differentiate between +0 vs -0 or to differentiate them based on sign as appropriate, with the default preserving the same behavior as occurs for regular format strings.

CC. @jeffhandley for secondary input -- This may be something we want to take to API review for secondary weigh in; but the original intent when we made the IEEE 754 compat fixes back in 3.0 was along the lines of the above.

[jeffhandley]

I support the approach @tannergooding described there, which as I understand it, keeps our current functional behavior intact and adjusts the docs to reflect the behavior we've had for several years now.

I don't think we need to float this past API review unless we're considering a change in functional behavior or a new concept.

[tannergooding]

Corresponding tests need to be added to Single and potentially Half

[Maximys]

Added by b4c6067a234b1b98b06367dfe9346b396f07bab4 commit

[Maximys]

@tannergooding , there is last failed test of my PR build and it's linked with Linux environment. Could you get me some instruction about how I can run build of .NET Runtime inside Docker container with Linux? I trying next command for build container, but it's failed:
docker run -v C:\runtime\:/runtime -w /runtime mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-18.04 ./build.sh

/runtime/eng/common/tools.sh: line 501: /runtime//.dotnet/dotnet: No such file or directory
Build failed with exit code 127. Check errors above.

I think it linked with invalid path for build("/runtime//.dotnet/dotnet"), but I'm not sure.

[tannergooding]

Sorry for the delay, I missed the last message and this never got put on my backlog

In general the instructions for building the runtime are here: https://github.com/dotnet/runtime/blob/main/docs/workflow/README.md

I personally don't use docker or any such related workflow, so I can't help on specifics around that.

[adamsitnik]

The docker instructions can be found here: https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/linux-instructions.md#build-using-docker

Here you can find the list of things that are required to build the repo on Linux: https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/linux-requirements.md#toolchain-setup

On Windows, the most convinient way of debugging a failing unit test is just adding the following snipplet to the begining of the test:

System.Diagnostics.Debugger.Launch(); // Windows asks you to choose the debugger, you point it to VS and just debug the failure

On Linux and macOS this API does not work, but you can simply add following code to the failing test:

while (!System.Diagnostics.Debugger.IsAttached)
{
    Thread.Sleep(TimeSpan.FromSeconds(1));
}

And use your IDE to attach debugger to a running process. In case of this particular case you need to run the following commands:

# to build CLR in Release and Libraries in Debug
./build.sh -subset clr+libs -rc Release
# To run the specific test
./dotnet.sh build ./src/libraries/System.Runtime.Numerics/tests/System.Runtime.Numerics.Tests.csproj /t:Test /p:xunitoptions="-method System.Numerics.Tests.ToStringTest.RunCustomFormatNumberScaling"
# To rebuild System.Private.CoreLib.dll without rebuilding the rest of the product
./build.sh clr.corelib+clr.nativecorelib+libs.PreTest -rc Release

Since the tests are failing I am going to "request changes" and turn it into DRAFT PR. Please let me know if you need more help.

[ghost]

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]

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.