Skip to content

Use u8 in more places #69995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
stephentoub merged 3 commits into from
Jun 7, 2022
Merged

Use u8 in more places #69995

stephentoub merged 3 commits into from
Jun 7, 2022

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented May 31, 2022

Primarily replacing Encoding.UTF8/ASCII.GetBytes("literal") with "literal"u8.ToArray()

Updates the pinned version of the C# compiler we're using so that we pick up recent updates to the u8 feature.

@ghost ghost assigned stephentoub May 31, 2022
@ghost ghost added the area-Meta label May 31, 2022
@ghost
Copy link

ghost commented May 31, 2022

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

Issue Details

Primarily replacing Encoding.UTF8/ASCII.GetBytes("literal") with "literal"u8.ToArray()

Updates the pinned version of the C# compiler we're using so that we pick up recent updates to the u8 feature.

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta
Milestone: -

@stephentoub stephentoub force-pushed the moreu8 branch 2 times, most recently from 07dd6d7 to af7ee78 Compare May 31, 2022 13:41
@stephentoub stephentoub requested a review from marek-safar as a code owner May 31, 2022 13:41
@stephentoub stephentoub force-pushed the moreu8 branch 2 times, most recently from 9ec81fb to 0acf732 Compare May 31, 2022 19:46
@vcsjones
Copy link
Member

vcsjones commented May 31, 2022 •
edited
Loading

One more, if you are interested in throwing it in this PR:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pem.iOS.cs

Line 14 in c12bea8

private static ReadOnlySpan<byte> PemBegin => new byte[] { 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, 0x42, 0x45, 0x47, 0x49, 0x4E, 0x20 };

You could probably get rid of that property entirely and change line 30 to:

if (rawData.IndexOf("-----BEGIN "u8) < 0)

(Assuming my understanding is correct that the "string"u8 syntax makes use of the same "Get ReadOnlySpan<byte> from the .TEXT section of the binary".

@stephentoub
Copy link
Member Author

stephentoub commented May 31, 2022 •
edited
Loading

@tannergooding, this pulls in an updated C# compiler, and there are a fair number of failures in UIntPtr-related tests, e.g.

runtime/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs

Line 1869 in 748dfb7

AssertBitwiseEqual(9223372036854775807.0, NumberBaseHelper<double>.CreateTruncating<nuint>(unchecked((nuint)0x7FFFFFFFFFFFFFFF))); 

    System.Tests.DoubleTests_GenericMath.CreateCheckedFromUIntPtrTest [FAIL]
      Assert.Equal() Failure
      Expected: 9.2233720368547758E+18
      Actual:   4294967295
      Stack Trace:
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(31,0): at System.Tests.DoubleTests_GenericMath.AssertBitwiseEqual(Double expected, Double actual)
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(1293,0): at System.Tests.DoubleTests_GenericMath.CreateCheckedFromUIntPtrTest()
        /_/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs(33,0): at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)
    System.Tests.DoubleTests_GenericMath.CreateTruncatingFromUIntPtrTest [FAIL]
      Assert.Equal() Failure
      Expected: 9.2233720368547758E+18
      Actual:   4294967295
      Stack Trace:
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(31,0): at System.Tests.DoubleTests_GenericMath.AssertBitwiseEqual(Double expected, Double actual)
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(1869,0): at System.Tests.DoubleTests_GenericMath.CreateTruncatingFromUIntPtrTest()
        /_/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs(33,0): at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)
    System.Tests.DoubleTests_GenericMath.CreateSaturatingFromUIntPtrTest [FAIL]
      Assert.Equal() Failure
      Expected: 9.2233720368547758E+18
      Actual:   4294967295
      Stack Trace:
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(31,0): at System.Tests.DoubleTests_GenericMath.AssertBitwiseEqual(Double expected, Double actual)
        /_/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs(1581,0): at System.Tests.DoubleTests_GenericMath.CreateSaturatingFromUIntPtrTest()
        /_/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs(33,0): at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)

There are some commented out tests there:

runtime/src/libraries/System.Runtime/tests/System/DoubleTests.GenericMath.cs

Lines 1871 to 1873 in 748dfb7

// https://github.com/dotnet/roslyn/issues/60714
// AssertBitwiseEqual(9223372036854775808.0, NumberBaseHelper<double>.CreateTruncating<nuint>(unchecked((nuint)0x8000000000000000)));
// AssertBitwiseEqual(18446744073709551615.0, NumberBaseHelper<double>.CreateTruncating<nuint>(unchecked((nuint)0xFFFFFFFFFFFFFFFF)));

with a comment to a Roslyn bug recently fixed, but I'm assuming the tests that aren't commented out are expected to continue working?

So far all the failures I've seen have been on Unix on mono.

@stephentoub stephentoub mentioned this pull request Jun 1, 2022
Merged
@runfoapp runfoapp bot mentioned this pull request Jun 1, 2022
Closed
@tannergooding
Copy link
Member

tannergooding commented Jun 1, 2022

Same as for #70087 (comment)

This looks like an issue similar to #69795.

I'd recommend disabling them just for Mono since its handling it incorrectly (looks like its always treating the target type as int/uint even on 64-bit systems)

@stephentoub
Use u8 in more places
bfca1a5 
Primarily replacing Encoding.UTF8/ASCII.GetBytes("literal") with "literal"u8.ToArray()
GrabYourPitchforks
GrabYourPitchforks reviewed Jun 6, 2022
View reviewed changes
@stephentoub stephentoub merged commit 0c4ee9e into dotnet:main Jun 7, 2022
@stephentoub stephentoub deleted the moreu8 branch June 7, 2022 17:42
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vcsjones vcsjones vcsjones left review comments

@gewarren gewarren gewarren left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

@tannergooding tannergooding tannergooding approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@stephentoub stephentoub

Labels

area-Meta

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@stephentoub @vcsjones @tannergooding @GrabYourPitchforks @gewarren