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

Spec out numeric IntPtr #6031

Merged
merged 18 commits into from
May 25, 2022
Merged

Spec out numeric IntPtr #6031

merged 18 commits into from
May 25, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 19, 2022

@jcouv jcouv self-assigned this Apr 19, 2022
For the predefined operators, the number of bits to shift is computed as follows:

- When the type of `x` is `int`, **`nint`** or `uint`, the shift count is given by the low-order five bits of `count`. In other words, the shift count is computed from `count & 0x1F`.
- When the type of `x` is `long`, **`nuint`** or `ulong`, the shift count is given by the low-order six bits of `count`. In other words, the shift count is computed from `count & 0x3F`.
Copy link
Contributor

@Rekkonnect Rekkonnect Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nuint belongs to the above category here #Resolved

Copy link
Member

@tannergooding tannergooding Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't nuint be masked to (sizeof(nuint) * 8) - 1 (0x3F on 64-bit and 0x1F on 32-bit)? If it's exclusively masked to 0x3F it will become non-deterministic for what the computed result is when over shifting is involved on a 32-bit platform.

Comment on lines 198 to 199
nint operator <<(nint x, nint count);
nuint operator <<(nuint x, nint count);
Copy link
Member

@tannergooding tannergooding Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't count be int for consistency with the other shift operators?

Or is this including the relaxations from #4666 ? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Bad search&replace ;-)

@jcouv jcouv marked this pull request as ready for review May 24, 2022 18:58
@jcouv jcouv requested a review from a team as a code owner May 24, 2022 18:58
@jcouv jcouv requested review from 333fred and cston May 24, 2022 18:58
- `IntPtr - int`
- `IntPtr -> int`
- `long -> IntPtr`
- `void* -> IntPtr`
Copy link
Member

@333fred 333fred May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this could overflow? Aren't these always the same size? #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# considered pointers as unsigned, so this can technically overflow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Chuck caught that. It's covered in the breaking changes document and in OverflowPointerConversion test.


### Breaking changes

One of the main impacts of this design is that `System.IntPtr` and `System.IntPtr` gain some built-in operators (conversions, unary and binary).
Copy link
Member

@333fred 333fred May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was one of these supposed to be UIntPtr? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Thanks


## 8.8 Unmanaged types

In other words, an __unmanaged_type__ is one of the following:
Copy link
Member

@cston cston May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unmanaged_type

Consider using italics rather than bold to match the existing spec and to avoid indicating a change. #Resolved


# 11 Expressions

#### 11.6.4.6 Better conversion target
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11.6.4.6 Better conversion target

Is there any change in this section? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be a change inside the ellipsis ("Specifically: ..."), but I didn't spell it out. I think the general rule is clear enough.


\[...] The number of expressions in the *argument_list* shall be the same as the rank of the *array_type*, and each expression shall be of type `int`, `uint`, **`nint`, `nuint`**, `long`, or `ulong,` or shall be implicitly convertible to one or more of these types.

#### 11.7.10.2 Array access
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11.7.10.2 Array access

Should this section include additions for nint and nuint? #Resolved


For the predefined operators, the number of bits to shift is computed as follows:
\[...]
- When the type of `x` is `nint` or `nuint`, the shift count is given by the low-order five bits of `count` on a 32 bits platform, or the lower-order six bits of `count` on a 64 bits platform.
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32 bits

Should these be "32 bit" and "64 bit" rather than "... bits" here and in the next sentence? #Closed

### Breaking changes

One of the main impacts of this design is that `System.IntPtr` and `System.UIntPtr` gain some built-in operators (conversions, unary and binary).
Those include `checked` operators, which means that some operators on those types will now throw when overflowing. For example:
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that some operators on those types

Perhaps "the following operators" to indicate this is the complete list of overflow changes. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the complete list. How about multiplication?

Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplication is not defined for IntPtr with C#10.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Thanks
Fixed

For the predefined operators, the number of bits to shift is computed as follows:
\[...]
- When the type of `x` is `nint` or `nuint`, the shift count is given by the low-order five bits of `count` on a 32 bits platform, or the lower-order six bits of `count` on a 64 bits platform.
The shift count is computed from `count & (sizeof(nint) * 8 - 1)`, which is `count & 0x1F` on a 32 bits platform and `count & 0x3F` on a 64 bits platform.
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count & (sizeof(nint) * 8 - 1), which is

The calculation using sizeof(nint) seems like an implementation detail. Consider omitting that part from the spec. #Closed


## 8.8 Unmanaged types

In other words, an **unmanaged_type** is one of the following:
Copy link
Member

@cston cston May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unmanaged_type

Should this be a single * or _ for italics? #Resolved

@jcouv jcouv merged commit 710d778 into dotnet:main May 25, 2022
@jcouv jcouv deleted the numeric-intptr branch May 25, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants