Skip to content

Remove unsafe code from number parsing #10397

Description

@danmoseley

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

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

Activity

  1. transferred this issue fromdotnet/coreclron Jan 31, 2020
  2. added this to the Future milestone on Jan 31, 2020
  3. felipepessoto commented on Jul 2, 2020

    @felipepessoto
    Contributor

    @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

  4. danmoseley commented on Jul 2, 2020

    @danmoseley
    ContributorAuthor

    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?

  5. safern commented on Jul 2, 2020

    @safern
    Member

    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.

  6. danmoseley commented on Jul 2, 2020

    @danmoseley
    ContributorAuthor

    @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".

  7. felipepessoto commented on Jul 2, 2020

    @felipepessoto
    Contributor

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

  8. safern commented on Jul 2, 2020

    @safern
    Member

    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

  9. felipepessoto commented on Jul 2, 2020

    @felipepessoto
    Contributor

    @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.

  10. safern commented on Jul 2, 2020

    @safern
    Member

    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>
    
  11. danmoseley commented on Jul 2, 2020

    @danmoseley
    ContributorAuthor

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

  12. safern commented on Jul 2, 2020

    @safern
    Member

    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.

  13. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Feb 14, 2021
  14. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Apr 24, 2021
  15. added
    in-prThere is an active PR which will close this issue when it is merged
    on Aug 5, 2026
  16. added a commit that references this issue on Aug 25, 2026
    cd25434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Cost:MWork that requires one engineer up to 2 weeksarea-System.Numericshelp wanted[up-for-grabs] Good issue for external contributorsin-prThere is an active PR which will close this issue when it is mergedreduce-unsafe

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions