Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider moving RuntimeHelper.GetSubArray to Array #29218

Closed
terrajobst opened this issue Apr 9, 2019 · 9 comments
Closed

Consider moving RuntimeHelper.GetSubArray to Array #29218

terrajobst opened this issue Apr 9, 2019 · 9 comments

Comments

@terrajobst
Copy link
Member

This came up as part of #28939.

Following the a range indexer binds to a method named Slice, we should also change GetSubArray() and put it on Array. It's a bit weird that it's not an instance member, but we can't make it an extension method on Array as the type isn't static. However, it still seems consistent as most array related methods are static.

 namespace System.Runtime.CompilerServices
 {
    public static class RuntimeHelpers
    {
-        public static T[] GetSubArray<T>(T[] array, Range range);
    }
 }
 namespace System
 {
     public class Array
     {
+        public static T[] Slice<T>(T[] array, int startIndex, int length);
     }
 }

@agocke @dotnet/ldm, any thoughts on this?

@jaredpar
Copy link
Member

jaredpar commented Apr 9, 2019

@davidwrighton as well as he chose the current location (guessing with some reason).

@agocke
Copy link
Member

agocke commented Apr 10, 2019

Honestly, I don't care. I'd keep it where it is, since that's the easiest solution.

@stephentoub
Copy link
Member

I'd keep it where it is

Regardless of where it lives, what about the Range => int, int change?

@agocke
Copy link
Member

agocke commented Apr 11, 2019

Slightly easier for the compiler if it's kept as range since we don't have to emit more code, but it doesn't really matter. I suppose we could do the same optimization for an inline range, but it just doesn't seem to matter vs allocation + copy.

@davidwrighton
Copy link
Member

Its in the current location, as when initially designed it was only going to be called from compiler generated code, and had no particular reason to be on System.Array. If its expected to be used by normal developers directly, then placing it on System.Array is perfectly reasonable. (In addition, the api wasn't quite like the other Slice methods as the method wasn't on the exact type, it could only have been placed on System.Array, not on the actual array types.)

I have no objection to moving it to System.Array, if it is to become like other methods.

@terrajobst
Copy link
Member Author

@dotnet/fxdc: anyone with strong opinions? We're running out of time and it seems neither of us feels super strongly. So my preference would be to leave it on RuntimeHelpers.

@terrajobst
Copy link
Member Author

terrajobst commented May 28, 2019

Video

Since we don't feel super strongly and the compiler would need to change, we're leaving it where it is.

@vladd
Copy link

vladd commented Aug 20, 2019

@terrajobst I'm trying to get as many as possible features from C# 8 working on .NET Framework 4.8. One of the obstacles is that I need to provide method RuntimeHelpers.GetSubArray in namespace System.Runtime.CompilerServices. This is possible by defining a class/method with appropriate names but leads to an unfortunate warning:

CSC : warning CS1685: The predefined type 'RuntimeHelpers' is defined in multiple assemblies in the global alias; using definition from 'Test, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'

From this perspective, wouldn't it make sense to reconsider the decision about leaving the method in RuntimeHelpers?

@terrajobst
Copy link
Member Author

terrajobst commented Aug 21, 2019

@vladd

We don't have plans to support C# 8 features on .NET Framework, so we didn't design the features with this constraint in mind. At this point, it's like way too late to change the compiler to accommodate this.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants