Enable CA1835-CA1841 #33270
Enable CA1835-CA1841 #33270
Conversation
pranavkm
force-pushed
the
prkrishn/fxcop-3
branch
2 times, most recently
from
728dc47
to
867c85c
Compare
pranavkm
requested review from
BrennanConroy,
halter73,
javiercn,
Pilchie,
Tratcher and
a team
as code owners
| ThrowIfDisposed(); | ||
|
|
||
| if (_bufferLimit.HasValue && _bufferLimit - Length < count) | ||
| if (_bufferLimit.HasValue && _bufferLimit - Length < buffer.Length) |
There was a problem hiding this comment.
Offset wasn't being used before when looking at these two checks. I'm going to assume that was a bug. @Tratcher
There was a problem hiding this comment.
Offset isn't important in this check, only the count/length. Offset was checked by ThrowArgumentException.
| @@ -145,13 +148,13 @@ public override async Task WriteAsync(byte[] buffer, int offset, int count, Canc | |||
|
|
|||
| if (BufferingEnabled) | |||
| { | |||
| if (_segmentWriteStream.Length + count > _maxBufferSize) | |||
There was a problem hiding this comment.
Another place we ignored offset before
There was a problem hiding this comment.
There was a validation check missing for offset, but count/length is the important part of this check.
| @@ -11,8 +11,8 @@ namespace Microsoft.AspNetCore.Http.Connections | |||
| { | |||
| internal static class ServerSentEventsMessageFormatter | |||
| { | |||
| private static readonly byte[] DataPrefix = { (byte)'d', (byte)'a', (byte)'t', (byte)'a', (byte)':', (byte)' ' }; | |||
| private static readonly byte[] Newline = { (byte)'\r', (byte)'\n' }; | |||
| private static readonly ReadOnlyMemory<byte> DataPrefix = new[] { (byte)'d', (byte)'a', (byte)'t', (byte)'a', (byte)':', (byte)' ' }; | |||
There was a problem hiding this comment.
I think the nice optimization with this only works for private static ReadOnlySpan<byte> blah => new byte[] {...};
There was a problem hiding this comment.
Sorry, I didn't follow - are you recommending changing this to a ReadOnlySpan and calling AsMemory at the callsite?
There was a problem hiding this comment.
Yeah
aspnetcore/src/Middleware/WebSockets/src/HandshakeHelpers.cs
Lines 19 to 20 in 4dc663a
There was a problem hiding this comment.
Unfortunately we can't go from Span -> Memory. I'll let this be
There was a problem hiding this comment.
@BrennanConroy Why is this writing to HttpResponse.Body instead of BodyWriter? If we did the latter, we could use the optimization you suggested.
There was a problem hiding this comment.
Because BodyWriter didn't exist when this code was written (plus it's SSE so...)
ghost
added this to the 
Contributes to #24055