Update SDK and use 'u8' string literals#41449
Conversation
BrennanConroy
requested review from
a team,
JamesNK,
Pilchie,
Tratcher,
dougbu,
halter73 and
wtgodbe
as code owners
src/DataProtection/Abstractions/test/DataProtectionCommonExtensionsTests.cs
Outdated
Show resolved
Hide resolved
src/DataProtection/Abstractions/test/DataProtectionCommonExtensionsTests.cs
Outdated
Show resolved
Hide resolved
| private static ReadOnlySpan<byte> MimePrefix => new byte[] { (byte)'"', (byte)'=', (byte)'?', (byte)'u', (byte)'t', (byte)'f', (byte)'-', (byte)'8', (byte)'?', (byte)'B', (byte)'?' }; | ||
| private static ReadOnlySpan<byte> MimeSuffix => new byte[] { (byte)'?', (byte)'=', (byte)'"' }; | ||
| private static ReadOnlySpan<byte> MimePrefix => "\"=?utf-8?B?"u8; | ||
| private static ReadOnlySpan<byte> MimeSuffix => "?=\""u8; |
There was a problem hiding this comment.
These changes do look like goodness. However, do they allocate more than the old form (given dotnet/roslyn#60644 and dotnet/csharplang#5983)❔
There was a problem hiding this comment.
No, they shouldn't allocate in these cases.
It's called dogfooding, people have to use a new feature in order to find the issues with it. Also, like you noted, this change makes some of our code look a lot nicer 😃 |
I also noticed a few icky extra casts and the variables that'll be useless once the feature's bugs are fixed. |
|
Sure, there is one ugly cast in CertificateManager, and a couple tests need a variable now, although the majority of the test changes actually look cleaner IMO, like https://github.com/dotnet/aspnetcore/pull/41449/files#diff-0fceab451ef913bdaa9272f539284c733c1892adc275414eb29250ff5b4b1facR71 we now use the same variable for the input and the expected output in the assert instead of duplicating the value. |
ghost
added this to the
5 participants
Updated SDK, this brings in a newer compiler with the new 'u8' string literal syntax. Sadly, there are some big breaking changes with the feature that will hopefully be fixed, tracking issue dotnet/roslyn#60644. And required some test code (and one area in product code, CertificateManager) to be updated.
The rest of the changes are to update our
ReadOnlySpan<byte> name => new byte[] { ... };code to use the new 'u8' feature.Also, folks likely need to update VS for the feature to light up.