StreamOnSqlBytes Read/Write on Spans #86674
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @roji, @ajcvickers Issue DetailsClose #69921
|
| // Adjust count based on data length | ||
| int count = Math.Min(buffer.Length, (int)(Length - offset)); | ||
|
|
||
| Span<byte> span = _rgbBuf!.AsSpan((int)offset, count); |
There was a problem hiding this comment.
Instead of the null-forgiving operator I'd prefer a Debug.Assert(_rgbBuf is not null), as in case it's actually null, then in CI the assert fails.
Same on other places.
Except it's really 100 % sure that it can't be null here, then ignore this comment.
There was a problem hiding this comment.
I copied all of ! from old array-based methods so I assume we are 100% sure
There was a problem hiding this comment.
Please remove the ! and add the assert. The ! was ok when this was in the Read method that was also checking IsNull and throwing, but now this is reachable from an internal method that has no such check.
There was a problem hiding this comment.
But internal method also has IsNull check under your comment about arguments order 🤔
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs
Outdated
Show resolved
Hide resolved
| // Adjust count based on data length | ||
| int count = Math.Min(buffer.Length, (int)(Length - offset)); | ||
|
|
||
| Span<byte> span = _rgbBuf!.AsSpan((int)offset, count); |
There was a problem hiding this comment.
Please remove the ! and add the assert. The ! was ok when this was in the Read method that was also checking IsNull and throwing, but now this is reachable from an internal method that has no such check.
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBytes.cs
Outdated
Show resolved
Hide resolved
|
@stephentoub do you have other feedback here? @ajcvickers is there someone from code owners that can review it ? |
|
All test failures are known |
Close #69921