Add ConcurrentBag/Queue.Clear #15292
Conversation
| <Compile Include="$(CommonTestPath)\System\Collections\IEnumerable.Generic.Serialization.Tests.cs"> | ||
| <Link>Common\System\Collections\IEnumerable.Generic.Serialization.Tests.cs</Link> | ||
| </Compile> | ||
| <Compile Include="ConcurrentBagTests.netstandard1.7.cs" /> |
There was a problem hiding this comment.
These are supposed to be named with netstandard1.7? It's somewhat confusing that in the ref file the members are conditionally included on #if netcoreapp11 but we include the tests for any netstandard1.7.
There was a problem hiding this comment.
I believe this is what's been done elsewhere. @weshaggard, is there new guidance?
There was a problem hiding this comment.
I assume this file is testing APIs that are only for netcoreapp correct? In which case this should use that naming convention. Also this test project currently only has netstandard build configurations (https://github.com/dotnet/corefx/blob/master/src/System.Collections.Concurrent/tests/Configurations.props#L4). You will need to add a netcoreapp configuration to correctly build this.
This might correctly compile today because we aren't correctly picking the netstanard targeting pack when building this but it is currently incorrect.
There was a problem hiding this comment.
I just filed issue https://github.com/dotnet/corefx/issues/15298 to fix netstandard test build configuration. Thanks for indirectly reminding and point it out to me.
There was a problem hiding this comment.
You will need to add a netcoreapp configuration to correctly build this.
@weshaggard, can you point to an example? I see, for example, that System.Collections has such new APIs added:
https://github.com/dotnet/corefx/blob/master/src/System.Collections/ref/System.Collections.cs#L60-L63
Which entry in its Configurations.props corresponds to netcoreapp?
https://github.com/dotnet/corefx/blob/master/src/System.Collections/src/Configurations.props
The closest one looks like "netcoreapp1.2corert", but that explicitly talks about corert.
|
@dotnet-bot test Innerloop CentOS7.1 Release Build and Test please (https://github.com/dotnet/corefx/issues/6302) |
| <Import Project="..\dir.props" /> | ||
| <PropertyGroup> | ||
| <AssemblyVersion>4.0.14.0</AssemblyVersion> | ||
| <AssemblyVersion>4.1.0.0</AssemblyVersion> |
There was a problem hiding this comment.
You don't need to version the contract any longer.
There was a problem hiding this comment.
You don't need to version the contract any longer.
Ok. So I can just undo all changes to this file?
There was a problem hiding this comment.
Cool. Why does this still exist then? Will it always be at this version number forever, or is it incremented only for some other reason, or is it just ignored?
There was a problem hiding this comment.
For now we have decided to keep the assembly versions fixed but we may need to update them again in the future, it isn't clear yet.
| bool System.Collections.ICollection.IsSynchronized { get { throw null; } } | ||
| object System.Collections.ICollection.SyncRoot { get { throw null; } } | ||
| public void Add(T item) { } | ||
| #if netcoreapp11 |
There was a problem hiding this comment.
The default build configuration is netcoreapp11 and so you don't actually need to do these #ifdef's any longer.
There was a problem hiding this comment.
@joperezr is in the process of updating the ref projects to only have netcoreapp and no netstandard configs any more.
There was a problem hiding this comment.
Is there an updated guide somewhere that details how to add new APIs?
There was a problem hiding this comment.
I've not updated the guide yet. I just filed https://github.com/dotnet/corefx/issues/15323 to track that. However as a quick summary now we will not be building netstandard refs in corefx any more for the contracts that are part of netstandard, those will be built in the dotnet/standard repo. Which means when editing those here the default will be netcoreapp and so you won't need these #ifdef's any longer and you will just naturally add them.
There was a problem hiding this comment.
That's helpful. Thanks for the info.
stephentoub
force-pushed
the
concurrent_clears
branch
from
db50f8e to
852a581
Compare
|
@weshaggard, does it look right this time? |
|
Thanks, @weshaggard, @ianhays, and @kouvel. |
|
Will this change be ported to full .Net Framework? |
|
It's not currently in the plan to port the Clear methods, flagging for port consideration |
…ears Add ConcurrentBag/Queue.Clear Commit migrated from dotnet/corefx@cdd1916
7 participants
Replaces #13976 and #14084. I initially cherry-picked the commits from those PRs, but I essentially had to rewrite the implementations due to the significant implementation changes that came in to ConcurrentBag and ConcurrentQueue in #14126 and #14254. Also cleaned up the tests @AlexRadch had added and added more.
Fixes https://github.com/dotnet/corefx/issues/2338
cc: @ianhays, @kouvel