Change Pin to take an optional integer offset #25770
Conversation
| @@ -53,7 +53,11 @@ protected override bool IsRetained | |||
|
|
|||
| public override unsafe Span<byte> Span => new Span<byte>((void*)_ptr, _length); | |||
|
|
|||
| public override unsafe MemoryHandle Pin() => new MemoryHandle(this, (void*)_ptr); | |||
| public override unsafe MemoryHandle Pin(int offset = 0) | |||
There was a problem hiding this comment.
What happens when offset is negative? Is it important to handle that case in more robust way?
There was a problem hiding this comment.
Similar case - offset is more than the length of the block?
There was a problem hiding this comment.
The NativeOwnedMemory is implementing OwnedMemory for testing purposes only so it isn't crucial for this implementation to validate offset.
It makes sense that for an array backed OwnedMemory, we can't support negative offset, but for NativeOwnedMemory, should a negative offset be allowed?
There was a problem hiding this comment.
I do not see the difference between the two cases. Negative offset does not make sense to either, I think.
|
Tests for the new parameter? |
|
Looks good after validating the parameter range. |
Since Pin is an abstract class, we don't really have direct tests for it. We already test the relevant scenarios through our Retain tests (the only place where we call Pin): I can't add tests for offsets that are negative or out of bounds because our Memory.Retain implementation guarantees that the offset is within range. |
|
Flagging this as a breaking change cc @marek-safar |
|
@ahsonkhan any update on this PR? |
I am working on figure out what caused the test failure. I should have an update by eod tomorrow. |
|
@ahsonkhan, was the issue not what I emailed you? "My guess is it’s due to this line: |
Yes, @stephentoub. That was the cause. I have fixed the implementation and added the tests. Thanks! This PR depends on the following for the CI to pass: dotnet/coreclr#15946 |
| { | ||
| unsafe | ||
| { | ||
| Retain(); | ||
| if (offset < 0 || (_array.Length > 0 && offset >= _array.Length)) throw new ArgumentOutOfRangeException(nameof(offset)); |
There was a problem hiding this comment.
I think this should rather be: if (offset < 0 || offset > _array.Length)
There was a problem hiding this comment.
Is it OK to get a pointer past the _array.Length for non-empty arrays?
Should I make the same change here?
https://github.com/dotnet/corefx/pull/25770/files#diff-eecad12eaed49ff891666e48f29f84b8R58
There was a problem hiding this comment.
It is ok to get a pointer that points right past the array, but not beyond it. It matches how offsets or index checks are done everywhere else. For example, the condition in Span.Slice is if ((uint)index > (uint)_count) ... error ....
| { | ||
| int* pointer = (int*)handle.Pointer; | ||
|
|
||
| GC.Collect(); |
There was a problem hiding this comment.
This test is useless. GC.Collect will not affect unmanaged pointer stored in a local.
I have merged your changes into the mirror PR already. If you would like to clean this up, do it in a follow up PR.
Resolves: https://github.com/dotnet/corefx/issues/25229
Depends on similar changes to be made in coreclr first for CI to pass - dotnet/coreclr#15410Depends on the following for the CI to pass: dotnet/coreclr#15946
cc @KrzysztofCwalina, @jkotas, @stephentoub, @pakrym, @davidfowl