Add new IntPtr/UIntPtr API surface #307

john-h-k · 2019-11-26T18:50:28Z

Implements https://github.com/dotnet/corefx/issues/20256

Delegates all implementations to [U]Int32/[U]Int64. Uses InvariantCulture for ToString to maintain backcompat for the old ToString() method

tannergooding · 2019-11-26T18:56:02Z

CC. @cston, @333fred

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

tannergooding · 2019-11-26T19:18:17Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

nit: I think the convention is to do ((nint)_value) rather than ((nint)this)

Do both forms generate same IL? We should make sure that the native code generated for these is good.

@john-h-k, could you get an assembly diff for the two forms?

I would hope the JIT folds them the same, but the latter ends up going through the explicit conversion API (vs directly converting the field).

We were already using the former for existing APIs (like ToString()).

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

tannergooding · 2019-11-26T19:20:39Z

The reference assembly also needs to be updated and tests added, but I don't believe we can do it in the same PR until after #92 is merged.

jkotas · 2019-11-26T19:27:06Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

There are number of places throughout CoreLib that assume IntPtr/UIntPtr are not comparable. We should fix them up as appropriate.

For example,

runtime/src/libraries/System.Private.CoreLib/src/System/Array.cs

Line 513 in 4f9ae42

// IntPtr/UIntPtr does not implement IComparable

should be fixed up.

@jkotas, do we have a rough list of these places?

It looks like 2 places in System.Array and no others for ELEMENT_TYPE_I or ELEMENT_TYPE_U (in managed code, I didn't check native):

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Array.cs#L515

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Array.cs#L1694

I did a quick scan of some other places and didn't spot anything that was keying off typeof(IntPtr or typeof(UIntPtr). Most places look to just key off x is IComparable.

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

src/libraries/System.Private.CoreLib/src/System/UIntPtr.cs

danmoseley · 2020-02-03T00:34:32Z

@tannergooding what is next here? Are you waiting on the assembly diff mentioned above? Or is this ready for re-review?

jkotas · 2020-02-03T01:23:24Z

This PR needs:

Matching change in reference assemblies
Tests
Addressing all feedback above (e.g. Add new IntPtr/UIntPtr API surface #307 (comment) )

tannergooding · 2020-02-03T01:23:28Z

It is waiting on the assembly diffs and the remaining feedback from Jan to be addressed. @john-h-k, please let us know if you need any assistance in getting this completed or if you have any questions.

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

Co-Authored-By: Tanner Gooding <[email protected]>

tannergooding

LGTM. Thanks for working through this and getting it done @john-h-k

tannergooding · 2020-04-20T15:49:30Z

@jkotas, would you be able to give this a quick review? It would be nice to get this in before the snap today so it can make the same preview as the compiler support for nint/nuint.

jkotas

LGTM

tannergooding · 2020-04-20T16:49:16Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

+        [NonVersionable]
+        public unsafe bool Equals(IntPtr other) => (nint)_value == (nint)other;
+
+        public unsafe override string ToString() => ((nint)_value).ToString(CultureInfo.InvariantCulture);


Test failures are because this is forcing InvariantCulture.

Either this needs to just mirror what int32/int64 do for their respective overloads or the test needs to be updated. I would guess the former is preferred and so this should just be ((nint)_value).ToString()

that is a breaking change as previously it was invariant by default right?

Yes, but it is also a breaking change to implement IFormattable (hence the breaking-change label on #21943).

I think it is the least confusing to just make this consistent moving forward and we can direct users to explicitly specify CultureInfo.InvariantCulture moving forward, if they want to be backwards compatible. Otherwise, you end up with behavior that differs from the other primitive types and how these overloads generally work together in general.

(Unless @jkotas or someone else has a different opinion here).

changed as this is already a breaking change, discussed w tanner somewhere else

* Add new IntPtr surface * Add new UIntPtr API surface * Add sequential layout to match [U]Int32 * Add interfaces and sequential layout * Add interfaces * Add namespaces * Add namespaces * Update UIntPtr.cs * Update IntPtr.cs * Change style * make non versionable, elide copy * fix style, elide copy, make non versionable * Fix syntax error * Fix style issues * Fix style issues * Update IntPtr.cs * Update UIntPtr.cs * Update ref assembly * Allow comparison of intptr/uintptr in Array * Fixed ELEMENT_TYPE cases, added tests based on Int32/UInt32 tests * Fixes * Update Array.cs * Update Array.cs * Update Array.cs * Update ArrayTests.cs * Update ArrayTests.cs * Update UIntPtrTests.cs * Update ArrayTests.cs * fix instance methods?? * fixes * Fixwa * fix tests * Add non versionables * fix compare methods * Fix comparison error * fix boundary * fix compares * fix maxvals * remove xunit buggy data * silly var name error * Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Co-Authored-By: Tanner Gooding <[email protected]> * Fix ToString Co-authored-by: Tanner Gooding <[email protected]>

* Add new IntPtr/UIntPtr API surface (#307) * Add new IntPtr surface * Add new UIntPtr API surface * Add sequential layout to match [U]Int32 * Add interfaces and sequential layout * Add interfaces * Add namespaces * Add namespaces * Update UIntPtr.cs * Update IntPtr.cs * Change style * make non versionable, elide copy * fix style, elide copy, make non versionable * Fix syntax error * Fix style issues * Fix style issues * Update IntPtr.cs * Update UIntPtr.cs * Update ref assembly * Allow comparison of intptr/uintptr in Array * Fixed ELEMENT_TYPE cases, added tests based on Int32/UInt32 tests * Fixes * Update Array.cs * Update Array.cs * Update Array.cs * Update ArrayTests.cs * Update ArrayTests.cs * Update UIntPtrTests.cs * Update ArrayTests.cs * fix instance methods?? * fixes * Fixwa * fix tests * Add non versionables * fix compare methods * Fix comparison error * fix boundary * fix compares * fix maxvals * remove xunit buggy data * silly var name error * Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Co-Authored-By: Tanner Gooding <[email protected]> * Fix ToString Co-authored-by: Tanner Gooding <[email protected]> * Remove the explicit IEquatable implementation from the IntPtr/UIntPtr reference API Co-authored-by: John <[email protected]>

maryamariyan added the area-System.Runtime label Nov 26, 2019

jkotas reviewed Nov 26, 2019

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs Outdated Show resolved Hide resolved

tannergooding reviewed Nov 26, 2019

View reviewed changes

jkotas reviewed Nov 26, 2019

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 26, 2019

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 26, 2019

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 26, 2019

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/UIntPtr.cs Outdated Show resolved Hide resolved

john-h-k added 17 commits April 5, 2020 22:36

Add new IntPtr surface

3d62768

Add new UIntPtr API surface

8ea1396

Add sequential layout to match [U]Int32

df81dbf

Add interfaces and sequential layout

e7e7c3e

Add interfaces

b846177

Add namespaces

fbbf549

Add namespaces

b0eaa01

Update UIntPtr.cs

0da4436

Update IntPtr.cs

2f1daed

Change style

38c0a1f

make non versionable, elide copy

e1b88b2

fix style, elide copy, make non versionable

c7f96a6

Fix syntax error

a91922f

Fix style issues

cdcef03

Fix style issues

27913f9

Update IntPtr.cs

dc00928

Update UIntPtr.cs

d6191eb

john-h-k added 14 commits April 18, 2020 22:16

Update ArrayTests.cs

b056313

Merge branch 'intptr-u' of github.com:john-h-k/runtime into intptr-u

af3abbd

fix instance methods??

2a6dbec

fixes

8dae896

Fixwa

794d9cb

fix tests

5c3f2e8

Add non versionables

98b3f57

fix compare methods

c6a4569

Fix comparison error

a58df5a

fix boundary

e758b05

fix compares

3dcccf0

fix maxvals

0377266

remove xunit buggy data

fe1711c

silly var name error

55d6f97

tannergooding reviewed Apr 20, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

48eb8d3

Co-Authored-By: Tanner Gooding <[email protected]>

tannergooding approved these changes Apr 20, 2020

View reviewed changes

jkotas approved these changes Apr 20, 2020

View reviewed changes

tannergooding reviewed Apr 20, 2020

View reviewed changes

john-h-k added 2 commits April 20, 2020 17:59

Fix ToString

529ca94

Merge branch 'intptr-u' of github.com:john-h-k/runtime into intptr-u

1a78c86

tannergooding merged commit 37fdae1 into dotnet:master Apr 21, 2020

tannergooding mentioned this pull request Apr 21, 2020

System.IntPtr and System.UIntPtr Improvements #21943

Closed

tannergooding mentioned this pull request Apr 21, 2020

[release/5.0-preview4] Add new IntPtr/UIntPtr API surface #35248

Merged

This was referenced Apr 21, 2020

Builds legs getting abandoned due to agent notification issues #35223

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Add new IntPtr/UIntPtr API surface #307

Add new IntPtr/UIntPtr API surface #307

Uh oh!

Conversation

john-h-k commented Nov 26, 2019

Uh oh!

tannergooding commented Nov 26, 2019

Uh oh!

Uh oh!

tannergooding Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

tannergooding Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tannergooding commented Nov 26, 2019

Uh oh!

jkotas Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Feb 3, 2020

Uh oh!

jkotas commented Feb 3, 2020

Uh oh!

tannergooding commented Feb 3, 2020

Uh oh!

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Apr 20, 2020

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

john-h-k Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

john-h-k Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tannergooding Nov 26, 2019 •

edited

Loading

tannergooding Nov 26, 2019 •

edited

Loading

jkotas Nov 26, 2019 •

edited

Loading