Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add CollectionsMarshal #26867

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Add CollectionsMarshal #26867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7faa40
Add CollectionsMarshal
benaadams Sep 24, 2019
143c599
Feedback
benaadams Sep 24, 2019
fa9345a
Feedback
benaadams Sep 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/System.Private.CoreLib/shared/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceType.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CoClassAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CollectionsMarshal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComDefaultInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComEventInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComImportAttribute.cs" />
Expand Down
4 changes: 2 additions & 2 deletions src/System.Private.CoreLib/shared/System/Collections/Generic/List.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ public class List<T> : IList<T>, IList, IReadOnlyList<T>
{
private const int DefaultCapacity = 4;

private T[] _items; // Do not rename (binary serialization)
private int _size; // Do not rename (binary serialization)
internal T[] _items; // Do not rename (binary serialization)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth a comment that these fields are internal only for the use of CollectionsMarshal and to avoid further growing the generated code for List? It's not something we normally do.

internal int _size; // Do not rename (binary serialization)
private int _version; // Do not rename (binary serialization)

#pragma warning disable CA1825 // avoid the extra generic instantiation for Array.Empty<T>()
Expand Down
21 changes: 21 additions & 0 deletions src/System.Private.CoreLib/shared/System/Runtime/InteropServices/CollectionsMarshal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;

namespace System.Runtime.InteropServices
{
/// <summary>
/// An unsafe class that provides a set of methods to access the underlying data representations of collections.
/// </summary>
public static class CollectionsMarshal
{
/// <summary>
/// Get a <see cref="Span{T}"/> view over a <see cref="List{T}"/>'s data.
/// Items should not be added or removed from the <see cref="List{T}"/> while the <see cref="Span{T}"/> is in use.
/// </summary>
Copy link
Member

@ahsonkhan ahsonkhan Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We should get in the habit of adding docs for parameters and return values (effectively all the xml fields that docs require). Something to consider going forward for new API additions (of course this applies to all future PRs and not just this one since we'd like to emphasize adding docs right in time and not accrue tech debt).

For example, see the following (which has param name and return value docs):
https://github.com/dotnet/dotnet-api-docs/blob/137fdd4f501ec90359833ad280064dbde43af7d3/xml/System.Collections/IList.xml#L213-L234
https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.icollection-1.contains?view=netframework-4.8

See the following for an example:
https://github.com/dotnet/corefx/blob/90f7cc01418c059f496528beeca77c5f5549e6fc/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L412-L432

cc @mairaw, @carlossanlop

Is there a way for us to detect and highlight the missing gaps in docs right in PRs to make it easier/discover-able for contributors?

public static Span<T> AsSpan<T>(List<T> list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need null argument validation here, don't we?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When writing the tests; I realised I'd prefer this arrangement

public static Span<T> AsSpan<T>(List<T>? list)
    => new Span<T>(list?._items, 0, list?._size ?? 0);

Not sure how people would feel about that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that better than list is null ? Span<T>.Empty : new Span<T>(list._items, 0, list._size)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the latter would have less null checks/etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier (at least for me) to reason about the code if written how @tannergooding suggested. I also would use default instead of Span<T>.Empty

public static Span<T> AsSpan<T>(List<T>? list)
    => list is null ? default : new Span<T>(list._items, 0, list._size);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we prefer Span<T>.Empty over default based on the existing usages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to see call sites of Span<T>.Empty than usage of default, but either works.

We have default being used in similar domain here:

coreclr/src/System.Private.CoreLib/shared/System/ArraySegment.cs

Line 170 in 163b30c

public static implicit operator ArraySegment<T>(T[] array) => array != null ? new ArraySegment<T>(array) : default;

There are only a few hits of Span<T>.Empty:
https://source.dot.net/#System.Private.CoreLib/shared/System/Span.cs,a02fdd2de3235cda,references

Although, a lot more for ReadOnlySpan<T>.Empty:
https://source.dot.net/#System.Private.CoreLib/shared/System/ReadOnlySpan.cs,47744f8b41c9226f,references

=> new Span<T>(list._items, 0, list._size);
}
}