Remove unsafe code from number parsing

[danmoseley]

There is heavy use of unsafe code in number.parsing.cs and calling code (eg at

runtime/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs

Line 255 in 110282c

private static unsafe bool TryParseNumber(ref char* str, char* strEnd, NumberStyles styles, ref NumberBuffer number, NumberFormatInfo info)

). This could be rewritten with ReadOnlySpan<char> to eliminate the char * resulting in safer code that is also easier to read.

Relates to dotnet/coreclr#17808

[ghost]

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

[felipepessoto]

@danmosemsft, @tannergooding, do you know a better strategy to edit/build/test it?

I was running these two command, but the first one takes a long time to run:

.\build.cmd -subset clr+libs -c Checked /p:BuildNative=false
D:\Repos\runtime\src\libraries\System.Runtime\tests> dotnet build /t:Test /p:Configuration=Checked

So, I'm currently doing this:

.\build.cmd -subset clr -c Checked /p:BuildNative=false

Copy "D:\Repos\runtime\artifacts\bin\coreclr\Windows_NT.x64.Checked\System.Private.CoreLib.dll"
To D:\Repos\runtime\artifacts\bin\testhost\net5.0-Windows_NT-Checked-x64\shared\Microsoft.NETCore.App\5.0.0\

D:\Repos\runtime\src\libraries\System.Runtime\tests> dotnet build /t:Test /p:Configuration=Checked

[danmoseley]

You're changing src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs and src/libraries/System.Runtime/tests right? @safern didn't we recently change things so that building src\libraries\System.Runtime\tests would also build System.Private.Corelib? In that case I would expect you could build clr+libs just once, and then iterate by doing dotnet build /t:build;test on System.Runtime\tests. Would that be right?

[safern]

Yeah we did but that doesn’t update the testhost because System.Private.CoreLib configuration has to match the whole runtime. What I do when I change System.Private.CoreLib and want to iterate on it to test again I run:

build.cmd clr.corelib+clr.nativecorelib+libs.pretest -rc <RuntimeConfig>

that will build Corelib, will run cross gen in it and will bin place it in the testhost.

[danmoseley]

@felipepessoto does that work for you?

I would not have figured that out. I wonder whether this is worth adding as an example to build -? and also documenting in docs/workflow/building/libraries/README.md. It's always good when we couch docs/help in terms of "if you want to do common thing X, then use this command line Y".

[felipepessoto]

I'll try it.
@safern, do you also know the easiest way to debug System.Private.CoreLib? Like the Number class

[safern]

I wonder whether this is worth adding as an example to build -? and also documenting in docs/workflow/building/libraries/README.md

Yeah makes sense, I'll add it.

I'll try it.
@safern, do you also know the easiest way to debug System.Private.CoreLib? Like the Number class

I usually just use the VS Test explorer and set breakpoints in System.Private.CoreLib source code

[felipepessoto]

@felipepessoto does that work for you?

I would not have figured that out. I wonder whether this is worth adding as an example to build -? and also documenting in docs/workflow/building/libraries/README.md. It's always good when we couch docs/help in terms of "if you want to do common thing X, then use this command line Y".

Worked very well. Now it takes 30~60 seconds to compile, much better than 12 minutes. Thanks.

[safern]

Worked very well. Now it takes 30~60

Btw, if you pass down the -build action to the script it will be faster since it will not do any of the -restore routines. i.e:

build.cmd clr.corelib+clr.nativecorelib+libs.pretest -build -rc <RuntimeConfig>

[danmoseley]

Shouldn't restore be super fast now, if it's already run?

[safern]

Shouldn't restore be super fast now, if it's already run?

It is, but in this case since you're using build.cmd it takes a few seconds to run the global tools install that arcade does for Tools.props etc, so it saves some seconds.