Implement Enumerable.ToHashSet() by JonHanna · Pull Request #13726 · dotnet/corefx

JonHanna · 2016-11-16T21:10:17Z

Closes #2323

A straight-forward implementation, forgoing any optimisations. HashSet's constructor already has some for some cases, and others could be wasteful unless not just the number of elements, but the number of elements that are distinct in terms of the equality comparer can be predicted.

JonHanna · 2016-11-16T21:10:58Z

  <Import Project="..\dir.props" />
  <PropertyGroup>
-    <AssemblyVersion>4.1.2.0</AssemblyVersion>
+    <AssemblyVersion>4.2.0.0</AssemblyVersion>


I gather it's ++minor on additions of public members. Am I correct?

stephentoub · 2016-11-16T21:15:39Z

        public static System.Collections.Generic.Dictionary<TKey, TElement> ToDictionary<TSource, TKey, TElement>(this System.Collections.Generic.IEnumerable<TSource> source, System.Func<TSource, TKey> keySelector, System.Func<TSource, TElement> elementSelector) { throw null; }
        public static System.Collections.Generic.Dictionary<TKey, TElement> ToDictionary<TSource, TKey, TElement>(this System.Collections.Generic.IEnumerable<TSource> source, System.Func<TSource, TKey> keySelector, System.Func<TSource, TElement> elementSelector, System.Collections.Generic.IEqualityComparer<TKey> comparer) { throw null; }
+        public static System.Collections.Generic.HashSet<TElement> ToHashSet<TElement>(this System.Collections.Generic.IEnumerable<TElement> source) { throw null; }
+        public static System.Collections.Generic.HashSet<TElement> ToHashSet<TElement>(this System.Collections.Generic.IEnumerable<TElement> source, System.Collections.Generic.IEqualityComparer<TElement> comparer) { throw null; }


@weshaggard, does any specialization need to be done for netstandard vs netcoreapp?

We had been surrounding new APIs with #if netcoreapp11 in the ref assemblies and tests were separated out into conditionally included _Foo_Tests.netcoreapp1.1.cs files, e.g. see #12226. Is that not necessary anymore?

I'll assume it still is, and update accordingly.

Yes it is still necessary for now. Please add a netcoreapp11 build configuration for these new APIs.

stephentoub · 2016-11-16T21:16:05Z

Implementation and tests LGTM. Thanks.

justinvp · 2016-11-16T22:03:10Z

            return d;
        }
+
+        public static HashSet<TElement> ToHashSet<TElement>(this IEnumerable<TElement> source) => source.ToHashSet(null);


Should the generic parameter be named TSource to match the naming used by ToList, ToArray, and ToDictionary? TElement is only used for the ToDictionary overloads with an elementSelector.

It was indeed ToDictionary that steered me wrong. Will change to TSource.

JonHanna · 2016-11-18T16:08:38Z

I'm missing something silly here. I've got ToHashSetTests.netcoreapp1.1.cs only coming in if '$(TargetGroup)' == 'netcoreapp1.1'. I've got that same condition setting netcoreapp11 and I've got ToHashSet in both ref and src if that directive is set.

What am I missing here?

mellinoe · 2016-11-19T01:04:14Z

Unfortunately it looks like Jenkins deleted all of the logs for that since the links all 404 for me.

@dotnet-bot Test this please

VSadov · 2016-11-19T01:35:56Z

implementation and tests LGTM.
I am not up to date with recent requirements in the area of build and versioning.

JonHanna · 2016-11-19T12:40:02Z

@mellinoe the error is "'IEnumerable' does not contain a definition for 'ToHashSet'…" and others that suggest that tests\ToHashSetTests.netcoreapp1.1.cs is being compiled against a version of System.Linq that didn't have netcoreapp11 defined when it compiled src\System\Linq\ToCollection.cs

ericstj · 2016-11-21T17:33:00Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

ericstj · 2016-11-21T18:04:46Z

@JonHanna check out the changes here: ericstj@a2274f9

weshaggard · 2016-11-21T18:15:16Z

+  <ItemGroup>
+    <Project Include="System.Linq.csproj" />
+    <Project Include="System.Linq.csproj">
+      <TargetGroup>netcoreapp1.1</TargetGroup>


Can you please flip and make netcoreapp1.1 the default and the netstandard16 the secondary configuration?

ericstj · 2016-11-21T18:30:51Z

When I built locally I also needed to change the name of the generic parameter, otherwise APICompat complained about TElement vs TSource

JonHanna · 2016-11-21T18:36:03Z

Ah. I had TElement and then changed to TSource when @justinvp pointed out the latter is more in keeping with when one or the other is used in the rest of the library, but I hadn't fixed the ref version to match. I've made the change there now.

And now it at least builds locally (though that was true of some other problematic pushes) so assuming it builds correctly here I'll sit down and learn what was going on, so I'll actually have some understanding the next time.

Now we play the waiting game…

Thanks.

JonHanna · 2016-11-21T18:39:48Z

@ericstj locally my coverage report shows ToHashSet() as being reachable but not covered. Is there something wrong with how I have:

  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp1.1'">
    <Compile Include="ToHashSetTests.netcoreapp1.1.cs" />
  </ItemGroup>

As well?

JonHanna · 2016-11-21T20:20:58Z

@dotnet-bot test this please.

JonHanna · 2016-11-21T20:38:11Z

@dotnet-bot test code coverage

ericstj · 2016-11-21T21:02:51Z

@JonHanna can you build src/System.Linq/pkg/System.Linq.pkgproj then address the error?

error : PackageIndex from D:\j\workspace\windows_nt_de---4526f5ff\pkg\Microsoft.Private.PackageBaseline\packageIndex.json is missing an assembly version entry for 4.2.0.0 for package System.Linq.  Please run /t:UpdatePackageIndex on this project to commit an update.

ericstj · 2016-11-21T21:04:28Z

WRT to coverage, I think you must be looking at the wrong report. Make sure to look at coverage for the netcoreapp1.1 run of the test.

JonHanna · 2016-11-21T21:35:40Z

Thanks that packages step goes some way to explain how things hang together and be less "magic just happens".
I only get one folder of coverage report from MSBuild /t:BuildAndTest /p:Coverage=true. Is there a parameter to pick builds?

ericstj · 2016-11-21T23:15:20Z

If you don't specify a project MSBuild picks the one file with a *proj extension, which means it doesn't pass any properties (like TargetGroup). If you want to run it on all configurations, specify the builds file. EG: MSBuild /t:BuildAndTest /p:Coverage=true System.Linq.Tests.builds.

JonHanna · 2016-11-21T23:21:21Z

Things are coming up green. Thanks again for your help, @ericstj

Closes #2323 A straight-forward implementation, forgoing any optimisations. HashSet's constructor already has some for some cases, and others could be wasteful unless not just the number of elements, but the number of elements that are distinct in terms of the equality comparer can be predicted.

VSadov · 2016-11-23T17:30:05Z

@weshaggard @ericstj - is this ok to merge now from the build/versioning prospective?

nawfalhasan · 2019-06-21T07:35:36Z

I dont have a strong opinion on this, just playing devils advocate, should ToHashSet throw on duplicates?

In favour:

ToDictionary throws on duplicates, so ToHashSet too. Just like ToDictionary cant choose which Value to pick, ToHashSet cant pick which reference to choose among the duplicates. Now that we have TryGet on HashSets, worth pondering.

Against:

new HashSet already discards duplicates, so developers expect ToHashSet to do the same.

I know this is shipped, but wondering..

Implement Enumerable.ToHashSet() Commit migrated from dotnet/corefx@e6b6ba1

dnfclas added the cla-already-signed label Nov 16, 2016

JonHanna commented Nov 16, 2016

View reviewed changes

stephentoub reviewed Nov 16, 2016

View reviewed changes

justinvp reviewed Nov 16, 2016

View reviewed changes

karelz added the area-System.Linq label Nov 16, 2016

karelz assigned JonHanna, VSadov and OmarTawfik Nov 16, 2016

JonHanna force-pushed the to_hash_set branch from 93a0210 to 3a47a40 Compare November 17, 2016 23:50

JonHanna changed the title ~~Implement Enumerable.ToHashSet()~~ [WIP] Implement Enumerable.ToHashSet() Nov 17, 2016

JonHanna force-pushed the to_hash_set branch 5 times, most recently from eae8abe to c46c1ca Compare November 18, 2016 15:38

JonHanna force-pushed the to_hash_set branch from c46c1ca to 50f4aa3 Compare November 19, 2016 19:40

weshaggard reviewed Nov 21, 2016

View reviewed changes

JonHanna force-pushed the to_hash_set branch from 5e0561b to cff7ce9 Compare November 21, 2016 21:30

JonHanna changed the title ~~[WIP] Implement Enumerable.ToHashSet()~~ Implement Enumerable.ToHashSet() Nov 21, 2016

JonHanna changed the title ~~Implement Enumerable.ToHashSet()~~ [WIP] Implement Enumerable.ToHashSet() Nov 22, 2016

JonHanna force-pushed the to_hash_set branch from cff7ce9 to b2991d9 Compare November 22, 2016 16:24

JonHanna force-pushed the to_hash_set branch from b2991d9 to c0e67b2 Compare November 23, 2016 02:17

JonHanna changed the title ~~[WIP] Implement Enumerable.ToHashSet()~~ Implement Enumerable.ToHashSet() Nov 23, 2016

VSadov merged commit e6b6ba1 into dotnet:master Nov 28, 2016

JonHanna deleted the to_hash_set branch November 28, 2016 19:14

karelz modified the milestone: 1.2.0 Dec 3, 2016

jamesqo mentioned this pull request Dec 4, 2016

Implement & expose SkipLast & TakeLast for Enumerable & Queryable #14186

Merged

This was referenced Dec 7, 2016

Expose ConcurrentDictionary GetOrAdd/AddOrUpdate overloads in netcoreapp1.1 #13828

Merged

Revert Linq Version change 4.1.2.0 → 4.2.0.0 *no merge* #14479

Closed

nawfalhasan unassigned OmarTawfik Jun 21, 2019

jamesqo mentioned this pull request Jan 31, 2020

Optimize new ToHashSet api. dotnet/runtime#19544

Closed

justinvp mentioned this pull request Jan 31, 2020

Revert System.Linq assembly version bump? dotnet/runtime#19666

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#13726 from JonHanna/to_hash_set

bdfe470

Implement Enumerable.ToHashSet() Commit migrated from dotnet/corefx@e6b6ba1

Conversation

JonHanna commented Nov 16, 2016

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!

stephentoub commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonHanna commented Nov 18, 2016

Uh oh!

mellinoe commented Nov 19, 2016

Uh oh!

VSadov commented Nov 19, 2016

Uh oh!

JonHanna commented Nov 19, 2016

Uh oh!

ericstj commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

ericstj commented Nov 21, 2016

Uh oh!

ericstj commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

ericstj commented Nov 21, 2016

Uh oh!

JonHanna commented Nov 21, 2016

Uh oh!

VSadov commented Nov 23, 2016

Uh oh!

nawfalhasan commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ericstj commented Nov 21, 2016 •

edited

Loading