Skip to content
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.

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

Use "..."u8 to simplify some ReadOnlySpan<byte> constructions #68334

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

stephentoub
Copy link
Member

I've focused only on easily-findable uses in src and only on non-allocating use.

I've focused only on src and only on non-allocating use.
@ghost
Copy link

ghost commented Apr 21, 2022

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

Issue Details

I've focused only on easily-findable uses in src and only on non-allocating use.

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta

Milestone: -


byte[]? cipherSuites = CipherSuitesPolicyPal.GetOpenSslCipherSuites(sslAuthenticationOptions.CipherSuitesPolicy, protocols, sslAuthenticationOptions.EncryptionPolicy);
Debug.Assert(cipherSuites == null || (cipherSuites.Length >= 1 && cipherSuites[cipherSuites.Length - 1] == 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't do the same trick Debug.Assert(cipherSuites == null || (cipherSuites[^1] == 0));?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still need the cipherSuites >= 1 && ..., but the cipherSuites.Length - 1 could be replaced by ^1.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I also verified all the ones that were previously numbers.

@am11
Copy link
Member

am11 commented Apr 21, 2022

It looks like concatenation syntax is not supported with ""u8, it would help shrinking the huge fields in
IcuLocaleData. 😢

@danmoseley danmoseley merged commit d122daa into dotnet:main Apr 21, 2022
@danmoseley
Copy link
Member

@stephentoub to make sure he sees the comment above about concatenation..

@stephentoub
Copy link
Member Author

I opened dotnet/roslyn#60896

@stephentoub stephentoub deleted the useu8ros branch April 21, 2022 23:58
@JamesNK
Copy link
Member

JamesNK commented Apr 22, 2022

Some of these files were synced to aspnetcore and are failing to build. What is the requirement to get this building?

See dotnet/aspnetcore#41317

@stephentoub
Copy link
Member Author

Some of these files were synced to aspnetcore and are failing to build

Sorry about that; I knew these would sync but forgot to deal with it before merging.

What is the requirement to get this building?

aspnetcore would need to move to a relatively recent compiler. Would you prefer I undo these changes for now, or do you want to upgrade aspnetcore?

@JamesNK
Copy link
Member

JamesNK commented Apr 24, 2022

No problem. Leave it in for now. There isn't a rush to merge the sync PR at the moment.

@ghost ghost locked as resolved and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants