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 ReadOnlySpan<byte> instead of byte arrays #35125

Merged

Conversation

vcsjones
Copy link
Member

A few more changes to use the compiler's ReadOnlySpan<byte> optimization trick instead of static byte arrays.

@ghost
Copy link

ghost commented Apr 17, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones
Notify danmosemsft if you want to be subscribed.

@tannergooding
Copy link
Member

@stephentoub, do we have an issue for an analyzer for this and sbyte (and the similar, but not quite as efficient, trick for non byte types)?

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

One cleanup comment, but otherwise LGTM.

SafeProvHandle hProv,
byte[] pbData,
ReadOnlySpan<byte> pbData,
int dwDataLen,
Copy link
Member

Choose a reason for hiding this comment

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

Since we're taking spans instead of entire arrays, does it make sense to clean up this call site by removing the dwDataLen parameter and inferring the value from the span length?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable; so changed. The callers of this always passed in array.Length anyway.

@stephentoub
Copy link
Member

do we have an issue for an analyzer for this and sbyte (and the similar, but not quite as efficient, trick for non byte types)?

Not yet implemented, but it's on our list: #33780

// Windows compat.
if (s_invalidEmptyOid.AsSpan().SequenceEqual(encodedOid))
if (invalidEmptyOid.SequenceEqual(encodedOid))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to just do:

if (encodedOid?.Length == 2 && encodedOid[0] == 6 && encodedOid[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.

Also a good idea. I changed the method's parameter from byte[] to ReadOnlySpan<byte> to remove the need for the null check and switched to AsnValueReader. The only caller of this method is here

if (rawData == null)
return null;
string contentTypeValue = PkcsHelpers.DecodeOid(rawData);

And already did the null check, so the implicit conversion doesn't come in to play.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also considered making a similar change to pSpecifiedDefaultParameters but since it is a negative condition it seemed to make things less readable.

@GrabYourPitchforks
Copy link
Member

One CI leg appears to be stalled - even on rerun. Not worried about it.

@GrabYourPitchforks GrabYourPitchforks merged commit e1fa787 into dotnet:master Apr 17, 2020
@vcsjones vcsjones deleted the span-statics-s.s.cryptography branch April 17, 2020 23:46
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.

5 participants