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

Updating Complex to implement INumberBase and ISignedNumber #68612

Merged
merged 5 commits into from
May 7, 2022

Conversation

tannergooding
Copy link
Member

No description provided.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 27, 2022

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@tannergooding
Copy link
Member Author

@marek-safar, @agocke, @davidwrighton

This is likely going to fail. I'm getting the following locally:

System.TypeLoadException : Virtual static method 'get_One' is not implemented on type 'System.Numerics.Complex' from assembly 'System.Runtime.Numerics, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.

In obj

  • .\artifacts\obj\System.Runtime.Numerics\Release\net7.0\PreTrim\System.Runtime.Numerics.dll looks correct
  • .\artifacts\obj\System.Runtime.Numerics\Release\net7.0\System.Runtime.Numerics.dll looks correct

In bin

  • artifacts\bin\System.Runtime.Numerics\Release\net7.0\System.Runtime.Numerics.dll looks correct
  • testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\System.Runtime.Numerics.dll looks correct

There is no copy of System.Runtime.Numerics.dll in the bin or obj folder for System.Runtime.Numerics.Tests

It's not clear if this is some trimming issue or VM metadata issue or something else. I'd likely lean towards the latter based on the assemblies I'm seeing having the relevant members defined, but this is problematic and blocking the PR.

@tannergooding
Copy link
Member Author

There isn't really anything special about Complex here. The only "notable" thing is that its not in S.P.Corelib where-as the primitives, Guid, DateTime, and NFloat are.

@agocke
Copy link
Member

agocke commented Apr 27, 2022

I can look at the trimmed DLL and see if it's missing anything if you can share it out

@tannergooding
Copy link
Member Author

VS reports the test is loading the assembly from .\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\System.Runtime.Numerics.dll and ildasm/dotPeek/ilspy all show that private static Complex System.Numerics.INumberBase<System.Numerics.Complex>.One { get; } still exists (as does public static readonly Complex One;, but that shouldn't be impactful).

Binary is here: System.Runtime.Numerics.dll.zip

But I have no clue if there is potentially something else missing as well.

@tannergooding
Copy link
Member Author

CC. @trylek as well, since this may be a VM issue

@agocke
Copy link
Member

agocke commented Apr 27, 2022

Might be missing a methodimpl here, + @jtschuster. I have to grab the pre-trimmed binary to be sure.

@tannergooding
Copy link
Member Author

Here's the obj folder (just for S.R.Numerics): System.Runtime.Numerics.zip

Contains both the trimmed and pre-trimmed output (the latter in the PreTrim folder).

@agocke
Copy link
Member

agocke commented Apr 27, 2022

Yup, MethodImpl is being dropped. This is a linker bug.

@tannergooding
Copy link
Member Author

Thanks much for looking!

@marek-safar
Copy link
Contributor

Yes, this is "expected" as linker work has not been completed. Tracking issue is dotnet/linker#2058

@tannergooding
Copy link
Member Author

This is ready for review now.

Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

Looks good!

@tannergooding tannergooding merged commit 7181c7f into dotnet:main May 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2022
@tannergooding tannergooding deleted the generic-math-complex branch November 11, 2022 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants