Remove version increment from Dictionary<K,V>.Remove overloads #18854
Conversation
| @@ -796,7 +796,6 @@ public bool Remove(TKey key) | |||
| } | |||
| _freeList = i; | |||
| _freeCount++; | |||
| _version++; | |||
There was a problem hiding this comment.
Is it worth a comment indicating why we aren't incrementing version in these places - without context, it might look like a mistake.
There was a problem hiding this comment.
As an aside I think we are inconsistent already. Looks like both TrimExcess() and EnsureCapacity() are missing version increments, even though they can invalidate an enumeration.
There was a problem hiding this comment.
Looks like both TrimExcess() and EnsureCapacity() are missing version increments, even though they can invalidate an enumeration.
Bug that we should fix?
There was a problem hiding this comment.
Both could silently change enumeration results in the worst case so i'd say bug and fix.
There was a problem hiding this comment.
|
With the new test system @A-And implemented, ~50 tests are now failing as a result of this change and will need to be disabled in coreclr before it can be merged (they will then need to also be fixed in corefx). |
|
To disable a test you can add it to the exclusion list. The expected format and a guide for testing can be found in the repo. |
|
@A-And thanks for the info. I've added the exclusions and the tests are re-running. I wasn't quite sure if I needed to add the parameters in the names but did so anyway. @danmosemsft if I ever get this PR and tests merged I'll probably pick up the trim one as well since I'm familiar with the area. |
|
@Wraith2 - the parameter names are tripping up the testing framework - you'd probably want just the test names, i.e. |
|
@A-And the test still is failing eg https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_checked_osx10.12_corefx_innerloop_prtest/164/ but seems excluded b49ee54#diff-2cb3a7742fa51cdfeb43b1cf4d901391R225 Can you please help us get this PR green? |
|
Alright, so there seems to be two thing here - The OSX and Ubuntu exclusions need to be added to https://github.com/dotnet/coreclr/blob/master/tests/CoreFX/TopN.CoreFX.x64.Unix.issues.json (this just got merged yesterday). Currently they're excluded on Windows, but not on OSX or Ubuntu. That was my bad - I should have mentioned it. See my comment for the Windows failing jobs. |
| "reason": "Assert.All() Failure: 1 out of 4 items in the collection did not pass.\r\n" | ||
| }, | ||
| { | ||
| "name": "System.Collections.Tests.Dictionary_Generic_Tests_SimpleInt_int_With_Comparer_WrapStructural_SimpleInt.IEnumerable_Generic_Enumerator_MoveNext_ModifiedAfterEnumeration_ThrowsInvalidOperationException(count: 1)", | ||
| "name": " System.Collections.Tests.Dictionary_IDictionary_NonGeneric_Tests.IEnumerable_NonGeneric_Enumerator_Reset_ModifiedAfterEnumeration_ThrowsInvalidOperationException", |
There was a problem hiding this comment.
This is being passed with an extra space in the beginning - the xunit runner parses it as a command arg and the test suite fails.
I.e. if you check https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_checked_windows_nt_corefx_innerloop_prtest/642/consoleFull you'll see:
error: unknown command line option: System.Collections.Tests.Dictionary_IDictionary_NonGeneric_Tests.IEnumerable_NonGeneric_Enumerator_Reset_ModifiedAfterEnumeration_ThrowsInvalidOperationException
|
We can merge now right? Still says WIP |
|
Could you please open a corefx issue to follow up and take advantage of this change in our corefx product code? |
Wraith2
changed the title
Removes the version increment from Remove operations
This addresses the coreclr side of the api change Add Dictionary<K,V>.Remove(predicate) with the intention of allowing removal of items from the dictionary while enumerating per direction from @jkotas . All collections tests and modified and new tests added in the related corefx PR.