Skip to content

Correct lowering of conversions between decimal and nint/nuint#40271

Merged
cston merged 2 commits intodotnet:features/NativeIntfrom
cston:decimal-conversions
Dec 13, 2019
Merged

Correct lowering of conversions between decimal and nint/nuint#40271
cston merged 2 commits intodotnet:features/NativeIntfrom
cston:decimal-conversions

Conversation

@cston
Copy link
Contributor

@cston cston commented Dec 10, 2019

Include missing conversions to/from 64-bit values when lowering conversions between decimal and nint / nuint.

Conversions within expression lambdas will be handled later.

Relates to #38821 (test plan for native ints)

@cston cston requested a review from a team as a code owner December 10, 2019 03:57
@cston cston requested review from 333fred and gafter December 10, 2019 05:52
@cston cston requested a review from a team December 11, 2019 16:46
@cston
Copy link
Contributor Author

cston commented Dec 12, 2019

@gafter, @dotnet/roslyn-compiler, please review, thanks.

base(other)
{
_underlying = other;
IsNativeInt = true;
Copy link
Member

@gafter gafter Dec 12, 2019

Choose a reason for hiding this comment

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

true [](start = 30, length = 4)

Please add this as an explicit parameter for clarity. #Closed

IL_0000: ldarg.0
IL_0001: call ""long decimal.op_Explicit(decimal)""
IL_0006: conv.i
IL_0007: newobj ""nint?..ctor(nint)""
Copy link
Member

@gafter gafter Dec 12, 2019

Choose a reason for hiding this comment

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

nint [](start = 25, length = 4)

Can we change the way this is displayed? nint doesn't exist at the IL level. #Closed

Copy link
Contributor Author

@cston cston Dec 12, 2019

Choose a reason for hiding this comment

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

When generating IL, we generate a set of tokens for the untranslated symbols referenced in IL (see CommonPEModuleBuilder.GetFakeSymbolTokenForIL). Those fake tokens are then translated later when emitting (see MetadataWriter.ResolveEntityHandleFromPseudoToken which eventually calls PEModuleBuilder.Translate) so that references to dynamic are converted to object and references to nint are converted to System.IntPtr, etc.

The IL visualizer in the test code (see ILBuilderVisualizer.ILBuilderToString) is handed the untranslated symbols and those are the symbols displayed. (Note that the IL visualizer reports references to dynamic rather than object as well.)

To fix the IL visualizer, we'd probably need to change CommonPEModuleBuilder.GetReferenceFromToken to call Translate on the symbol before returning it. That would change the display of dynamic and nint and perhaps other symbols however.

Logged #40340.


In reply to: 357284988 [](ancestors = 357284988)

Copy link
Member

@tmat tmat Dec 12, 2019

Choose a reason for hiding this comment

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

VisualizeRealIL does that. #Closed

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@cston cston merged commit f22ad6e into dotnet:features/NativeInt Dec 13, 2019
@cston cston deleted the decimal-conversions branch December 13, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants