Allow collection expressions for 'System.Collections.ObjectModel' collection types #111093
Conversation
…jectModel' collection types dotnet#110161
|
Note regarding the |
1 similar comment
|
Note regarding the |
dotnet-policy-service
bot
added
the
community-contribution
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Show resolved
Hide resolved
Enhance ReadOnlyCollection with documentation and updates The `EditorBrowsable` attribute is replaced with a `SuppressMessage` attribute to clarify the intended usage of these methods. Additionally, the condition for checking if input values are empty is updated from `values.Length <= 0` to `values.IsEmpty` for improved readability and performance.
Eliminated the `using System.ComponentModel;` directive, reducing dependencies and potentially simplifying the code structure.
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
Outdated
Show resolved
Hide resolved
| /// <returns>A new <see cref="ReadOnlySet{T}"/> containing the specified values.</returns> | ||
| [Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0301:Simplify collection initialization", Justification = "This method simplifies ReadOnlySet<T> initialization, so you cannot simplify collection initialization inside it.")] | ||
| public static ReadOnlySet<T> CreateSet<T>(params ReadOnlySpan<T> values) => | ||
| values.IsEmpty ? ReadOnlySet<T>.Empty : new ReadOnlySet<T>((HashSet<T>)[.. values]); |
There was a problem hiding this comment.
Using the spread operator on sets doesn't appear to produce optimal code:
Consider handrolling the factory?
There was a problem hiding this comment.
Using the spread operator on sets doesn't appear to produce optimal code:
Using Count is not entirely appropriate because the enumeration might contain duplicates, and in some cases, there could be many of them. This would result in unnecessary memory allocation.
Additionally, I think that the compiler is capable of optimizing while (enumerator.MoveNext()) loops effectively.
There was a problem hiding this comment.
I will replace the loop with a manual, but should I use the Count property?
There was a problem hiding this comment.
Additionally, I think that the compiler is capable of optimizing while (enumerator.MoveNext()) loops effectively.
The C# compiler shouldn't be emitting use of an enumerator here, just as it doesn't when foreach'ing over a span, but for some reason is. @cston, @RikkiGibson?
There was a problem hiding this comment.
Looks like a roslyn bug. I see some bits in foreach lowering which are handling the span case and "rewriting as for", which are missed in the spread element lowering.
There was a problem hiding this comment.
I will replace the loop with a manual, but should I use the Count property?
Presumably you mean .Length? The input is a ReadOnlySpan.
There was a problem hiding this comment.
Presumably you mean
.Length? The input is a ReadOnlySpan.
Yes! I mean the Length property. Is it correct to use Length here or not? Sometimes a source may contain many duplicates. This will result in unnecessary memory allocation.
Should I rewrite the loop to a manual for, or will the compiler error be fixed?
There was a problem hiding this comment.
Yes! I mean the
Lengthproperty. Is it correct to useLengthhere or not? Sometimes a source may contain many duplicates. This will result in unnecessary memory allocation.
Good point. The compiler doesn't pre-size the HashSet instances either so we should do the same here.
Should I rewrite the loop to a manual for, or will the compiler error be fixed?
A handwritten factory is fine I think, it doesn't fundamentally change the amount of IL being generated.
The `CreateCollection<T>` method in the `ReadOnlyCollection` class had its suppression attribute removed, addressing a style warning related to collection initialization while maintaining its functionality. Similarly, the `CreateSet<T>` method in the `ReadOnlySet` class also had its suppression attribute removed, preserving its original functionality.
| /// <typeparam name="T">The type of elements in the collection.</typeparam> | ||
| /// <param name="values">The span of values to include in the collection.</param> | ||
| /// <returns>A new <see cref="ReadOnlySet{T}"/> containing the specified values.</returns> | ||
| public static ReadOnlySet<T> CreateSet<T>(params ReadOnlySpan<T> values) => |
There was a problem hiding this comment.
What is the reason for this method to be ReadOnlyCollection.CreateSet<T>() instead of ReadOnlySet.Create<T>() in a new static class?
The later is much more user friendly.
There was a problem hiding this comment.
The builder methods aren't for users. They are for compilers to build collection expressions.
The compiler only requires static method in non-generic type.
There was a problem hiding this comment.
What is the reason for this method to be
ReadOnlyCollection.CreateSet<T>()instead ofReadOnlySet.Create<T>()in a new static class? The later is much more user friendly.
I could replace ReadOnlyCollection.CreateSet<T>() with the ReadOnlySet.Create<T>() method if that's more correct.
There was a problem hiding this comment.
I could replace
ReadOnlyCollection.CreateSet<T>()with theReadOnlySet.Create<T>()method if that's more correct.
That's not what's approved in API review. Questions around the names should go back to the original issue.
There was a problem hiding this comment.
I could replace
ReadOnlyCollection.CreateSet<T>()with theReadOnlySet.Create<T>()method if that's more correct.That's not what's approved in API review. Questions around the names should go back to the original issue.
It says that methods can be created in one class or separate ones... #110161 (comment)
We could add another type (e.g. System.Collections.ObjectModel.Collection or one non-generic type per collection (like we do in immutable).
Close #110161