Issue #15622 New overload Dictionary.Remove by WinCPP · Pull Request #18109 · dotnet/corefx

WinCPP · 2017-04-08T06:44:42Z

New overload Dictionary<TKey, TValue>.Remove(TKey, out TValue) that
ouputs value associated with the key that is being removed.

Fixes #15622.

Depends on dotnet/coreclr PR #10203

@karelz @danmosemsft @safern @jkotas Please review.

WinCPP · 2017-04-08T07:05:43Z

@danmosemsft @safern @karelz @jkotas Does this change require adding files such as ApiCompatBaseline.uapaot.txt that I had done for PR #16507 for issue #16274?

safern · 2017-04-08T10:53:42Z

Yes it will need the BaselineCompat file for uapaot. Still you will need to port the implementation to dotnet/corert and once that merges and gets consumed in corefx we can remove that baseline, but before that it is needed.

safern · 2017-04-08T10:57:24Z

+            dictionary.Add(missingKey, CreateTValue(34251));
+            Assert.True(dictionary.Remove(missingKey, out value));
+            Assert.Equal(count, dictionary.Count);
+            Assert.Equal(CreateTValue(34251), value);


Instead of calling CreateTValue twice you could create a TValue variable to store it and add it and then compare against that. Also... instead of using a random seed could we use count as a seed or something like that?

We should also verify that TryGetValue returns false for the key after removal.

Used CreateTValue once.

Actually this was to create and check the value consistency... Changed random seed to count that is passed to the test method.

Added TryGetValue

safern · 2017-04-08T10:58:07Z

+        public void Dictionary_Generic_RemoveKey_DefaultKeyNotContainedInDictionary(int count)
+        {
+            Dictionary<TKey, TValue> dictionary = (Dictionary<TKey, TValue>)GenericIDictionaryFactory(count);
+            TValue value = CreateTValue(12345);


Same thing here about the seed.

Fixed using count passed to the method.

safern · 2017-04-08T11:00:29Z

+            else
+            {
+                Assert.Throws<ArgumentNullException>(() => dictionary.Remove(default(TKey)));
+                Assert.Equal(CreateTValue(12345), value);


What is this assert testing?

Oops... The lines 204 and 205 were supposed to be like this...

Assert.Throws<ArgumentNullException>(() => dictionary.Remove(default(TKey), out outValue)); Assert.Equal(refValue, outValue);

Where outValue was supposed to be assigned with refValue and after the exception the assert was to ensure that the outValue hasn't changed from refValue. ~~This is based on the knowledge that the Remove(K, V) method (just like existing Remove(K) method) doesn't assign default(TValue) to output parameter in the Null Key case.~~

Changed the code...

Edited:
Sorry, I have struck out the factually incorrect information that I quoted above. I think I need to change the Remove(K, V) API implementation to set the output parameter value to default(TValue) in case it is about to throw exception due to Null key. Not assigning default to output parameter had broken the convention that callee always sets output parameter on all paths, either to expected value or to default. [I am not able to recollect why I didn't set it to default(TValue) in first place, but I do remember that I had done it consciously based on something that I had observed at that time...] Keeping @jkotas @danmosemsft in loop for the change that I will be doing at this place.

safern · 2017-04-08T11:03:15Z

+                TValue value = default(TValue);
+
+                if (!dictionary.ContainsKey(missingKey))
+                    dictionary.Add(missingKey, CreateTValue(5341));


Instead of checking if the key exists before adding, we could probably use dictionary.TryAdd

Fixed by using TryAdd.

safern · 2017-04-08T11:03:28Z

+                if (!dictionary.ContainsKey(missingKey))
+                    dictionary.Add(missingKey, CreateTValue(5341));
+                Assert.True(dictionary.Remove(missingKey, out value));
+                Assert.Equal(CreateTValue(5341), value);


Why is this assert needed?

Removed :-)

WinCPP · 2017-04-09T06:03:20Z

@safern could you please help me with the path for BaselineCompat file... couldn't locate file with this name in CoreFx / CoreClr repos... Thank you!

safern · 2017-04-10T17:38:21Z

@safern could you please help me with the path for BaselineCompat file... couldn't locate file with this name in CoreFx / CoreClr repos... Thank you!

You will need to run msbuild src\System.Collections\src\System.Collections.csproj /p:TargetGroup=uapaot /p:BaselineAllApiCompatError=true to update the file.

WinCPP · 2017-04-11T15:58:10Z

@safern hmmm to clarify a bit more. I'm not having local compilation issues. I was referring to any ApiCompatBaseline.uapaot.txt file that needs to be manually edited ( @stephentoub ) had pointed me to one as mentioned in previous comment (the LazyInitializer task that I had done) ... Between, I ran the above command but it created following two files which are not checked in into the repo. So it seems that in this particular case there is no ApiCompatBaseline.uapaot.txt or equivalent file to be edited and checked in for the new overload that I added into CoreClr...? Thanks.

src/System.Collections/src/ApiCompatBaseline.txt
src/System.Diagnostics.Debug/src/ApiCompatBaseline.txt

danmoseley · 2017-04-13T05:22:47Z

@safern for your q.

safern · 2017-04-13T22:55:38Z

@WinCPP you don't need the baseline anymore since the corert changes should be now ingested as your PR on that side was merged 2 days ago. So you will just need to do 2 things to fix CI checks.

Fetch the latest changes in dotnet/corefx and rebase your branch on top of it.
Follow the next steps (See how we do it in other test classes):

Add a new file to the test project Dictionary.Generic.Tests.netcoreapp.cs
Move the tests you added to that file.
Include it in the csproj with a netcoreapp condition:
- <Compile Include="Generic\Dictionary\Dictionary.Generic.Tests.netcoreapp.cs" Condition="'$(TargetGroup)'=='netcoreapp'" />

The second has to be done because the new API that you added is not available in desktop so when building the netfx tests we get a failure saying it can't find that API. So we should only include your tests when testing for netcoreapp.

WinCPP · 2017-04-13T23:04:14Z

@safern Thanks for the steps. I will do them. Between, a bit unrelated question. Whenever I get latest changes from dotnet/master for CoreFx using "Update from dotnet/master" button on GitHub client, it creates a merge commit on my branch and adds up the files on changed file list on PR even thought they are just branch updates. Hence @stephentoub had suggested me to squash everything on branch. Are there some special steps that you all follow to not get a Merge commit on the branch... but still have genuine code changes as valid commits?

WinCPP · 2017-04-14T10:56:50Z

Thanks @safern. The checks are passing. Please review.

safern

Thanks @WinCPP for your contribution.

danmoseley · 2017-04-14T18:47:49Z

@WinCPP

Whenever I get latest changes from dotnet/master for CoreFx using "Update from dotnet/master" button on GitHub client, it creates a merge commit on my branch and adds up the files on changed file list on PR even thought they are just branch updates. Hence @stephentoub had suggested me to squash everything on branch. Are there some special steps that you all follow to not get a Merge commit on the branch... but still have genuine code changes as valid commits?

I don't know about GitHub client, what all (?) of us do is use git.exe, something like

git checkout master
git pull
git checkout mybranch
git rebase master

if you have changes you would either commit them to mybranch first or stash them temporarily out the way.

For your own changes, you can usually just commit as you please and we (almost always) merge your PR to master with squash so it doesn't matter. Also typically people just code review the whole thing not commit by commit. That's what most people do most times.

Sometimes it's worth having clean logical separate commits though

I like to squash typoes locally but that's just me.
If the change is logically one change but easier to review as a series of commits. You'd call that out in the PR. We'll ultimately squash before merging into master.
If there is code cleanup or reformatting it's nice to segregate that from real changes. Again we'll probably squash before merging.
If they are completely distinct but related changes. For example I did some cleanup changes where I removed redundant define XYZ then removed define ABC. These made sense as separate commits. In such cases we would typically merge the PR as a merge commit so the distinction is preserved forever. That is easier to read later, to bisect or revert individually if needed.

danmoseley · 2017-04-14T18:48:20Z

Dunno if @stephentoub has anythign else to say about that.

danmoseley · 2017-04-14T18:49:07Z

BTW @WinCPP are you planning to pick up another work item? Need any help selecting? As you know our focus is on Milestone=2.0 right now.

stephentoub · 2017-04-14T18:51:17Z

Dunno if...

Sounds about right. I don't use the GitHub standalone client tool, instead using either the GitHub integration with VS or the command-line. For rebasing, I use the command-line, similar to what @safern outlined. And I basically always rebase rather merge when working on a change.

danmoseley · 2017-04-14T18:56:11Z

Personally I use a combination of git.exe and SourceTree because then I know what's actually going on. I generally don't use VS for this either as it abstracts a bit high for me also. Each to their own.

safern · 2017-04-14T18:56:28Z

I like having different upstreams (one my fork and one dotnet/corefx repo) and I never pull from my master fork branch. I always do:

git checkout MyIssueBranch
git fetch dotnet master
git rebase dotnet/master

That will place the commits on MyIssueBranch on top of dotnet/master changes one by one and will merge changes or will ask to solve merge conflicts. Whenever I want to solve conflicts I first squash everything to one commit (to solve conflicts once) and I use VS tool to solve merge conflicts and the command line for everything else.

WinCPP · 2017-04-15T01:28:58Z

@safern @danmosemsft @stephentoub thank you for the inputs on Git usage. I will go by pure git.exe instead of GitHub windows client and try as the latter is a bit confusing, especially while working with forks.

@danmosemsft I am working on #17118 which should be done in a day (I have left one comment there about optimization) and then will get back to XPath test cases that we previously discussed. Is there something that you would want me to pick up that might help you guys with 2.0...?

Aah... I have inputs on #8578 now which is Milestone 2.0 task. I will finish it off as well...

danmoseley · 2017-04-15T04:21:11Z

@WinCPP anything 2.0 is great really depends on your interests. if you need help scoping how how complex something might be just ping on the issue.

* Issue #15622 New overload Dictionary.Remove

ViktorHofer · 2017-04-17T22:04:18Z

@WinCPP

Some 2.0 tasks where help is appreciated:

* Issue #15622 New overload Dictionary.Remove

WinCPP · 2017-04-18T20:48:26Z

@ViktorHofer @karelz I can take up #16640 (2.0 issue) mentioned above before I go back to the performance data collection for PR #18435 (non-2.0 issue)...

Please note that #16510 is already closed...

ViktorHofer · 2017-04-18T20:51:33Z

@WinCPP I assigned you to #16640

dnfclas added the cla-already-signed label Apr 8, 2017

WinCPP force-pushed the issue-15622 branch from 8836f78 to 8256eec Compare April 8, 2017 09:26

safern reviewed Apr 8, 2017

View reviewed changes

karelz added the area-System.Collections label Apr 8, 2017

karelz assigned WinCPP, ianhays and safern Apr 8, 2017

WinCPP force-pushed the issue-15622 branch from fe5e15a to 1a097f6 Compare April 9, 2017 05:41

WinCPP mentioned this pull request Apr 9, 2017

CoreFx #15622 New overload Dictionary.Remove(K, out V) dotnet/corert#3270

Merged

WinCPP added 2 commits April 14, 2017 04:38

Issue #15622 New overload Dictionary.Remove

7ef94b0

Issue #15622 New overload Dictionary.Remove

7c8de87

WinCPP force-pushed the issue-15622 branch from ad35bc9 to 7c8de87 Compare April 13, 2017 23:10

Limit tests for Remove(K, out V) to netcoreapp

4402509

safern approved these changes Apr 14, 2017

View reviewed changes

safern merged commit 6080a38 into dotnet:master Apr 14, 2017

karelz modified the milestone: 2.0.0 Apr 15, 2017

WinCPP added a commit to WinCPP/corefx that referenced this pull request Apr 15, 2017

Issue #15622 New overload Dictionary.Remove (dotnet#18109)

ba648bd

* Issue #15622 New overload Dictionary.Remove

safern mentioned this pull request Apr 16, 2017

Add TryAdd/Remove extension methods in CollectionExtensions #18091

Merged

WinCPP added a commit to WinCPP/corefx that referenced this pull request Apr 18, 2017

Issue #15622 New overload Dictionary.Remove (dotnet#18109)

94eebdb

* Issue #15622 New overload Dictionary.Remove

WinCPP deleted the issue-15622 branch March 8, 2018 17:10

Conversation

WinCPP commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WinCPP commented Apr 8, 2017

Uh oh!

safern commented Apr 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP commented Apr 9, 2017

Uh oh!

safern commented Apr 10, 2017

Uh oh!

WinCPP commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Apr 13, 2017

Uh oh!

safern commented Apr 13, 2017

Uh oh!

WinCPP commented Apr 13, 2017

Uh oh!

WinCPP commented Apr 14, 2017

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Apr 14, 2017

Uh oh!

danmoseley commented Apr 14, 2017

Uh oh!

danmoseley commented Apr 14, 2017

Uh oh!

stephentoub commented Apr 14, 2017

Uh oh!

danmoseley commented Apr 14, 2017

Uh oh!

safern commented Apr 14, 2017

Uh oh!

WinCPP commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Apr 15, 2017

Uh oh!

ViktorHofer commented Apr 17, 2017

Uh oh!

WinCPP commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Apr 18, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

WinCPP commented Apr 8, 2017 •

edited

Loading

WinCPP Apr 9, 2017 •

edited

Loading

WinCPP commented Apr 11, 2017 •

edited

Loading

WinCPP commented Apr 15, 2017 •

edited

Loading

WinCPP commented Apr 18, 2017 •

edited

Loading

ViktorHofer commented Apr 18, 2017 •

edited by danmoseley

Loading