Repository navigation
Remove unsafe code from number parsing #10397
Description
Activity
- addeduntriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner
on Feb 26, 2020 - added and removeduntriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner
on Jun 23, 2020 - addedhelp wanted[up-for-grabs] Good issue for external contributors[up-for-grabs] Good issue for external contributors
on Jun 23, 2020 @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=CheckedSo, 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
You're changing
src/libraries/System.Private.CoreLib/src/System/Number.Parsing.csandsrc/libraries/System.Runtime/testsright? @safern didn't we recently change things so that buildingsrc\libraries\System.Runtime\testswould also buildSystem.Private.Corelib? In that case I would expect you could buildclr+libsjust once, and then iterate by doingdotnet build /t:build;teston System.Runtime\tests. Would that be right?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.
@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 indocs/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".I'll try it.
@safern, do you also know the easiest way to debug System.Private.CoreLib? Like the Number classI 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 classI usually just use the VS Test explorer and set breakpoints in System.Private.CoreLib source code
@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 indocs/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.
Worked very well. Now it takes 30~60
Btw, if you pass down the
-buildaction to the script it will be faster since it will not do any of the-restoreroutines. i.e:build.cmd clr.corelib+clr.nativecorelib+libs.pretest -build -rc <RuntimeConfig>Reacted by Felipe PessotoShouldn't restore be super fast now, if it's already run?
Shouldn't restore be super fast now, if it's already run?
It is, but in this case since you're using
build.cmdit takes a few seconds to run the global tools install that arcade does forTools.propsetc, so it saves some seconds.- addedCost:MWork that requires one engineer up to 2 weeksWork that requires one engineer up to 2 weeks
on Jan 15, 2021 - ghost addedin-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged
on Feb 14, 2021 - ghost removedin-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged
on Apr 24, 2021 - addedin-prThere is an active PR which will close this issue when it is mergedThere is an active PR which will close this issue when it is merged
on Aug 5, 2026 - added a commit that references this issue
on Aug 25, 2026
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
ReadOnlySpan<char>to eliminate thechar *resulting in safer code that is also easier to read.Relates to dotnet/coreclr#17808