-
Notifications
You must be signed in to change notification settings - Fork 344
Renamed several types in System.Buffers.Primitives #1343
Conversation
cc: @ahsonkhan, @davidfowl, @pakrym |
@@ -97,45 +97,45 @@ public void CopyTo(Span<T> span) | |||
Span.CopyTo(span); | |||
} | |||
|
|||
public void CopyTo(Memory<T> memory) | |||
public void CopyTo(Buffer<T> memory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about the name of the arguments, especially for public surface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will change them
|
||
public IMemoryList<byte> Rest => _next; | ||
|
||
public int WrittenByteCount => _written; | ||
|
||
ReadOnlyMemory<byte> IReadOnlyMemoryList<byte>.First => Memory; | ||
ReadOnlyBuffer<byte> IReadOnlyMemoryList<byte>.First => Memory; | ||
|
||
IReadOnlyMemoryList<byte> IReadOnlyMemoryList<byte>.Rest => _next; | ||
|
||
// TODO: maybe these should be renamed to no conflict with OwnedMemory<T>.Length? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment still apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, it's conflicting with Length. But I renamed the name of the class to OwnedBuffer
@@ -5,10 +5,10 @@ | |||
using System.Collections.Sequences; | |||
|
|||
namespace System.Buffers { | |||
public interface IReadOnlyMemoryList<T> : ISequence<ReadOnlyMemory<T>> | |||
public interface IReadOnlyMemoryList<T> : ISequence<ReadOnlyBuffer<T>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to rename the interface as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Thanks.
@@ -5,7 +5,7 @@ | |||
|
|||
namespace System.Buffers | |||
{ | |||
public class OwnedNativeMemory : OwnedMemory<byte> | |||
public class OwnedNativeMemory : OwnedBuffer<byte> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change OwnedNativeMemory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. thanks.
536c65f
to
fea1778
Compare
@@ -3,7 +3,7 @@ | |||
|
|||
namespace System.Buffers | |||
{ | |||
public sealed class OwnedArray<T> : OwnedMemory<T> | |||
internal sealed class OwnedArray<T> : OwnedBuffer<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no new APIs added to OwnedArray and so no need for the type to be public.
} | ||
void ThrowArgHelper() | ||
{ | ||
throw new ArgumentOutOfRangeException(); | ||
} | ||
#endregion | ||
|
||
public static OwnedBuffer<T> Create(ArraySegment<T> segment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-)
@@ -1,42 +1,43 @@ | |||
// Copyright (c) Microsoft. All rights reserved. | |||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
|||
using System.Buffers; | |||
using System.Diagnostics; | |||
|
|||
namespace System.Runtime | |||
{ | |||
internal class MemoryDebuggerView<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BufferDebuggerView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. Thanks
@@ -250,14 +250,14 @@ public static ReadableBuffer Create(byte[] data, int offset, int length) | |||
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.length); | |||
} | |||
|
|||
var buffer = new OwnedArray<byte>(data); | |||
OwnedBuffer<byte> buffer = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the type be stated explicitly in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not about guidelines/conventions. var buffer = data would declare and assign T[] buffer local. We need buffer to be OwnedBuffer<byte> not byte[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit ctor via cast via declaration?
Wouldn't var buffer = (OwnedBuffer<byte>)data;
be better or expose a Create(T[] array)
function or people might either get confused how to create or start using (due to not working out how to create the buffer)
var buffer = Create(new ArraySegment<T>(array, 0, array.Length);
Which would be inefficent
You are walking into a mine field... asp.net and .net team have different guidelines. 🍿 |
fea1778
to
6152c23
Compare
I like the new names. |
Renamed:
Moved all the types to System.Buffers namespace
Move BufferPool to System.Buffers
Made the following types internal:
The names might not be final, but I think they move us closer to where we want to be