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 for DefaultExponent #33820

Merged
merged 4 commits into from
Mar 27, 2020
Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Mar 19, 2020

Just something I noticed while perusing code.

/cc @bartonjs

OpenSSL handles this condition itself.
@ghost
Copy link

ghost commented Mar 19, 2020

Hello @bartonjs!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@GrabYourPitchforks
Copy link
Member

Something strikes me as weird about using ref byte in a p/invoke signature to represent a buffer. When I look at such a signature I'd expect "oh, this one value is being read from or written to." Contrast this against byte*, which more canonically represents "this is the start of some block of contiguous data."

@vcsjones
Copy link
Member Author

@GrabYourPitchforks hm, yeah I suppose I could see that. There is prior art for both cases. Since "System.Net.Http.Functional.Tests" saved this from getting merged, is the preference that I change it?

@GrabYourPitchforks
Copy link
Member

If auto-merge failed, may as well improve it. Didn't want to hold up an already approved PR just for this though. :)

@bartonjs
Copy link
Member

All of the byte[] to span P/Invoke changes I've seen have used ref byte.

@stephentoub Do you have thoughts or feelings for byte* vs ref byte in P/Invokes?

@stephentoub
Copy link
Member

Do you have thoughts or feelings for byte* vs ref byte in P/Invokes?

byte* is likely tad cheaper / less magic, assuming it's the only thing that would cause a stub to need to be created. But no strong preference.

@vcsjones
Copy link
Member Author

vcsjones commented Mar 20, 2020

@bartonjs @GrabYourPitchforks So, uh change it?

While I have y'all here, is there a meaningful difference between:

fixed (byte* x = &MemoryMarshal.GetReference(foo)) {
}

and

fixed (byte* x = foo) {
}

Is the former just from the times before GetPinnableReference was a thing?

@stephentoub
Copy link
Member

is there a meaningful difference between

Yes. The latter includes length check that returns null if the length is 0:

ref T ret = ref Unsafe.NullRef<T>();
if (_length != 0) ret = ref _pointer.Value;
return ref ret;

The former does not:
public static ref T GetReference<T>(ReadOnlySpan<T> span) => ref span._pointer.Value;

@bartonjs bartonjs merged commit 5e4d6c0 into dotnet:master Mar 27, 2020
@vcsjones vcsjones deleted the minor-span-fixup branch March 27, 2020 16:08
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 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.

None yet

5 participants