Skip to content

Update API to use Native-Sized Integers #387

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

Closed
thargy opened this issue Dec 30, 2020 · 1 comment · Fixed by #392
Closed

Update API to use Native-Sized Integers #387

thargy opened this issue Dec 30, 2020 · 1 comment · Fixed by #392
Assignees
Labels
area-Prospective enhancement New feature or request
Milestone

Comments

@thargy
Copy link
Contributor

thargy commented Dec 30, 2020

Summary of feature

C# 9 added support for native-sized integers as a drop-in replacements for IntPtr (and UIntPtr). The new native-sized intger types, nint and nuint, were added specifically to improve the experience of coding for interop scenarios, e.g. by low-level library authors, of which Silk.Net is a prime example. By changing our references to IntPtr and UIntPtr to nint and nuint respectively, consumers using C#9.0 will be pointed towards the new types and will benefit from the improvements provided.

Further, Silk.Net internal development will benefit by being less verbose.

Comments

After reviewing the implementation, the 2 new types are effectively implemented as syntatic sugar over IntPr and UIntPtr, with a new attribute NativeIntegerAttribute marking instances of the type for code compiled using C#9.0+. As such, library consumers on an older version of C# should continue to see the API as exposing IntPtr and UIntPtr and there shouldn't be any breaking changes introduced.

Following this change, code such as:

    public unsafe IntPtr Add(IntPtr a, IntPtr b)
    {
        return (IntPtr)(void*)((long)a + (long)b);
    }
  • which assumes a 64-bit architecture, can instead be written safely as:
	public nint Add(nint a, nint b) => a + b;

So long as the consumer is using C#9.0+.

Note this code can be called using IntPtr's due to implicit casting, e.g.:

	public IntPtr Test(IntPtr a, IntPtr b) => Add(a, b);

Will continue to work, allowing the API to be backwards compatible.

It would be useful to introduce the change in a preview build, after initial careful testing, to see if it causes any issues for existing consumers.

Alternatives

It is also plausible to not make this change at all. Consumers can then optionally perform the casting themselves. The only downside is the reduced discoverability of the new modern types, as there shouldn't be any real performance impact of casting, for example, the following is valid:

    public unsafe IntPtr AddP(IntPtr a, IntPtr b) => (IntPtr)(void*)((long)a + (long)b); 
    public nint Add(nint a, nint b) => AddP(a,b);
@thargy thargy added the enhancement New feature or request label Dec 30, 2020
@thargy
Copy link
Contributor Author

thargy commented Dec 30, 2020

Note, as per the proposal, the following pointer members are not exposed on the corresponding native-sized integer:

// constructors
// arithmetic operators
// implicit and explicit conversions
public static readonly IntPtr Zero; // use 0 instead
public static int Size { get; }     // use sizeof() instead
public static IntPtr Add(IntPtr pointer, int offset);   // use + operator instead
public static IntPtr Subtract(IntPtr pointer, int offset); // use - operator instead
public int ToInt32(); // cast to int instead
public long ToInt64(); // cast to long instead
public void* ToPointer();

However, implicit casting (actually there is no cast performed in reality as the implementation is syntatic sugar) comes to the rescue in most use cases, e.g.:

nint a = IntPtr.Zero; // Existing code works when assigning to nint
((IntPtr)a).ToPointer(); // A cast is needed here - though is is removed on compilation.

Because of these changes then any existing C#9.0 code that expects to receive a pointer from the API, but instead gets the new native-sized integer, and is using implicit typing, the code will fail to compile if they try to access any of ToInt32(), ToInt64() and ToPointer(). The fix for these is trivial, and C#9.0 is new enough that there is unlikely to be a large technical debt of code written at that lang-level.

e.g.

// Implicit typed a used to be IntPtr, but is now nint
var a = APIMethodPreviousReturnedIntPtr();
var b = a.ToInt32(); // No longer compiles

// Or direct access to return value
b = APIMethodPreviousReturnedIntPtr().ToInt32(); // No longer compiles

IntPtr c = APIMethodPreviousReturnedIntPtr(); // Implicit 'cast'
b = c.ToInt32(); // Succeeds
b = ((IntPtr)APIMethodPreviousReturnedIntPtr()).ToInt32(); // Succeeds

(Similar for out and ref parameters though these are less likely to be implicitly typed)

Conclusion

C#9.0 was only released last month, and Silk.NET 2.0 is still in preview with breaking changes. As such this is the ideal time to make the change with minimal (if any) impact. A future change would be more difficult once 2.0 goes RTM, as more production code is likely to be developed against it using C#9.0; also anyone who upgrades their existing projects to C#9.0 will experience the breaking changes at the point of upgrade. It would be much less impactful if they are doing so at the same time as upgrading to v.2.0 of the library.

@Perksey Perksey added this to the 2.0 milestone Jan 1, 2021
@Perksey Perksey self-assigned this Jan 1, 2021
Perksey added a commit that referenced this issue Jan 2, 2021
* nints and start of Vulkan Function Tables (closes #387)

* Missed a bit

* Add {RESULT}

* Vulkan function tables

* XR function tables and build fixes

* Fix build

How many times am I gonna do this?

* Use Regex for faster Substitution

* Update release notes, update NativePackages

Co-authored-by: Kai Jellinghaus <contact@kaij.tech>
Co-authored-by: Craig Dean <thargy@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Prospective enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants