Fix BigInt parsing logic #214
Conversation
lahma
commented
Dec 13, 2021
lahma
commented
- fix various issues found while integrating into Jint
- add netstandard2.1 target
- use spans for literal parsing
- add System.Memory as dependency
* add netstandard2.1 target * use spans for literal parsing * add System.Memory as dependency
src/Esprima/Scanner.cs
Outdated
| // ensure we get positive number | ||
| number = ("0" + number.ToString()).AsSpan(); | ||
| } | ||
| #if !NETFRAMEWORK && !NETSTANDARD2_0 |
There was a problem hiding this comment.
| #if !NETFRAMEWORK && !NETSTANDARD2_0 | |
| #if NETSTANDARD2_1_OR_GREATER |
Is that what you meant?
There was a problem hiding this comment.
Does it include everything like NETCOREAPP31 onwards? standard vs modern net5/6 can be confusing.
src/Esprima/Esprima.csproj
Outdated
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="all" /> | ||
| <PackageReference Include="System.Memory" Version="4.5.4" /> |
There was a problem hiding this comment.
1- What's coming from it?
2- Would it be removed if TFMs included netcoreapp3.1 ... net6.0?
There was a problem hiding this comment.
Basically the AsSpan, might be good idea to refine targets and remove dep if possible
|
Can you add the |
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="all" /> | ||
| <PackageReference Include="System.Memory" Version="4.5.4" Condition=" '$(TargetFramework)' != 'netstandard2.1'"/> |
There was a problem hiding this comment.
Is it required for netstandard2.1 ?
There was a problem hiding this comment.
Seems that compilation works without it.
| if (c > 7 && Character.IsHexDigit(c)) | ||
| { | ||
| // ensure we get positive number | ||
| number = ("0" + number.ToString()).AsSpan(); |
There was a problem hiding this comment.
ToString() on spans, maybe this method should just take the original string, that had to be converted as a span.
There was a problem hiding this comment.
Span is part of read source, this is a special case when we need to materialize in order to get parsing to work correctly.
Pushed a commit. |
|
Not this PR, I feel like we need a custom class for each Literal type. Would also prevent boxing. |
|
Jint now passes its bigint test suite with these changes. |
