Moves ByReference to shared CoreLib #21633
Conversation
| @@ -11,26 +11,31 @@ namespace System | |||
| // around lack of first class support for byref fields in C# and IL. The JIT and | |||
| // type loader has special handling for it that turns it into a thin wrapper around ref T. | |||
| [NonVersionable] | |||
| internal readonly ref struct ByReference<T> | |||
| internal readonly ref partial struct ByReference<T> | |||
There was a problem hiding this comment.
Nit: Does it need to be partial?
There was a problem hiding this comment.
CoreRT has an additional attribute on this type so I made it partial for that reason
There was a problem hiding this comment.
That attribute was a quick workaround. It should not be needed anymore. And if it is still needed I would rather put it under #if PROJECTN ifdef so that it is easy to remove. We do not bother refactoring files into partial classes just to apply temporary TODO workarounds.
There was a problem hiding this comment.
Yeah, maybe it's not needed anymore. I've put it in dotnet/corert#3000. It should have been cleaned up when the Project N side was fixed up to support TypedReference. We'll see in the next integration and then I'll deal with it.
There was a problem hiding this comment.
@jkotas Making this a readonly struct broke the contract of the class library with the code generator backend in ProjectN. I'm getting asserts in some UTC macros in precheckin. Since I don't see value in sharing this implementation detail, and I doubt I can get anyone from the UTC team to look at it, what's the cleanest way to make CoreRT opt out of participating in the sharing scheme for this file? (I don't think ridding this file with ifdefs would look great.)
We can revisit if not having ByReference<T> marked readonly is ever a problem.
There was a problem hiding this comment.
I don't think ridding this file with ifdefs would look great
Ifdefs as workarounds for bugs are fine. I do not think we need to be doing refactorings to make workarounds look pretty. Workarounds for bugs are never pretty. We always had some of these workaround PROJECTN ifdefs and kept adding and removing them as necessary.
Marking `ByReference as readonly makes the IL generated for Span and friends leaner. The IL for Span has extra locals when ByReference is not marked readonly.
|
@jkotas why this move?Some new feature? |
|
@MarcoRossignoli This change contributes to https://github.com/dotnet/coreclr/issues/17904 and related issues. For engineering efficiency, we want to have all CoreLib code that is shareable between CoreCLR, Mono and CoreRT runtimes to be in the shared CoreLib partition. |
|
@Anipik Is the CoreLib mirror running? |
|
I made some changes to the mirror to add a unidirectional mirror for mono..that might have caused mirror to stop. I am out of office with no way to connect to the remote machine. I will resolve the issue in the evening or tomorrow |
|
No worries. Thanks! |
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Commit migrated from dotnet/coreclr@cc68e4a
No description provided.