Adding ValueTuple and ITuple to corlib in CoreCLR #8695
Conversation
|
do you want to add the new exposed types to the file https://github.com/dotnet/coreclr/blob/master/src/mscorlib/model.xml to ensure the rewriter will include it? |
|
CC @jkotas |
|
LGTM. |
|
Nice. Do you want to consider cherry-picking the changes from dotnet/corefx#14187 here as well? They were rejected since the corefx Besides that, LGTM. |
| @@ -8,12 +8,14 @@ namespace System.Numerics.Hashing | |||
|
|
|||
| internal static class HashHelpers | |||
| { | |||
| public static readonly int RandomSeed = Guid.NewGuid().GetHashCode(); | |||
There was a problem hiding this comment.
This does not seem to be actually used.
Also, could you please change this to be initialized with new Random().Next(Int32.MinValue, Int32.MaxValue) instead? CoreCLR and CoreRT are initializing the random seed via NewGuid() already - it would be nice to do it just once per process.
There was a problem hiding this comment.
Thanks for catching that. I accidentally copied over the code that used the random seed. Fixed.
|
|
||
| if (!(other is ValueTuple)) | ||
| { | ||
| throw new ArgumentException(Environment.GetResourceString("ArgumentException_ValueTupleIncorrectType", this.GetType().ToString()), nameof(other)); |
There was a problem hiding this comment.
Could you please add the getter for the resource to src\mscorlib\corefx\SR.cs to so that this file can be exactly same as the copy in CoreRT?
|
(Once this goes through and propagates to corefx, you will also need to update the packaging in corefx to use the forwarders to corelib where applicable.) |
|
@jkotas Yes, updating type forwarders (and also adding tests for |
jcouv
force-pushed
the
tuple-coreclr
branch
2 times, most recently
from
eb12c23
to
a8c3e5d
Compare
|
@jamesqo If you wouldn't mind taking another look. I added your change from dotnet/corefx#14187. Thanks |
| @@ -8608,6 +8608,14 @@ | |||
| <Member Name="GetHashCode" /> | |||
| <Member Name="ToString" /> | |||
| <Member MemberType="Property" Name="Item1" /> | |||
| <Member Status="ImplRoot" Name="System.Collections.IStructuralComparable.CompareTo(System.Object,System.Collections.IComparer)" /> | |||
There was a problem hiding this comment.
@tarekgh Thanks for the modelgen tool. I copied the bits that I think are useful here, but I don't really know how to validate.
In particular, I'm surprised that these pre-existing Tuple APIs weren't in the model.xml. Is adding them correct?
There was a problem hiding this comment.
The corelib pruning tool automatically keeps interface implementations around. It is not necessary to mention them explicitly.
There was a problem hiding this comment.
to validate it, after compiling S.P.Corelib.dll look inside and make sure all new exposed APIs are already there and not get removed by the IL re-writer.
There was a problem hiding this comment.
I've spot checked the S.P.Corelib.dll and the new APIs appear there.
I've left the interface implementations in the model.xml. Let me know if they need to be removed, or ok to leave. Thanks
|
BTW: I am moving the EditorBrowsableAttribute to CoreLib in #8703 |
|
@jcouv I have moved the EditorBrowsableAttribute to CoreLib. You can uncomment it. |
|
@jcouv LGTM |
|
Thanks. Rebased and removed the commit that was removing EditorBrowsable (so the attribute is back in TupleExtensions.cs). |
* Adding ValueTuple * Adding ITuple * Porting equality comparer optimization from corefx PR dotnet/coreclr#14187 Commit migrated from dotnet/coreclr@2d49c2c
ValueTuple types are supporting the C#7.0 tuples feature. They have been implemented as a standalone CoreFx package, but should be moved down into the various corlibs (mscorlib/desktop and mscorlib/mono are already done) to be consistent with
System.Tupleand can be used by other APIs.This PR does not contain
ITupleyet, but I will push it shortly into this same PR.It is an interface in
CompilerServicesnamespace used for dynamic pattern matching (in a future version of C#). It applies toSystem.TupleandSystem.ValueTupletypes. It was implemented in mscorlib/desktop, and I'm in the process of implementing it in other corlibs.The main purpose for opening this PR at this point is to raise two questions:
ValueTuplego?System.Tuple? I could not find it.FYI @tarekgh @VSadov