Introduce ScopedBuffer #415
Conversation
| #if NET8_0 || NETSTANDARD2_1 | ||
| #if NET8_0_OR_GREATER || NETSTANDARD2_1 |
There was a problem hiding this comment.
Thanks for fixing this. I searched and couldn't find any other instances.
There was a problem hiding this comment.
Yep, also searched -- and only one I found!
| var keyRefs = ArrayPool<byte>.Shared.Rent(count); | ||
| Span<byte> keyRefs = stackalloc byte[count]; // <= 255 |
There was a problem hiding this comment.
Could we convert the comment into a Debug.Assert(count <= 255) instead? It provides a little more safety and is clearer as far as documenting intent.
There was a problem hiding this comment.
I updated the comment to be clearer (count is an unsigned byte, with a safe range) -- no need for an assert. The comment saves a second when auditing the stackalloc call sites.
| @@ -193,17 +195,11 @@ namespace MetadataExtractor.IO; | |||
| } | |||
| else | |||
There was a problem hiding this comment.
Do we need the else block any more? Looks like the size check could be moved into a ternary conditional and the two blocks collapsed into one, as you've done elsewhere in the PR.
There was a problem hiding this comment.
unified the logic across all the readers. much tidier.
|
Addressed all feedback, and double checked against regressions. Merged! |
|
Looks great, thanks. FYI I will be less online over the next week but will do my best to review any PRs. |
This PR extracts the ScopedBuffer work from #406
Local regression testing shows no diffs.
@drewnoakes Ready for review.