- Notifications
You must be signed in to change notification settings - Fork768
Disable implicit conversion from PyFloat to .NET integer types#1343
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
7c4a6b5 toefe84c2Compare
tminka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
test_uint64_conversion should similarly raise a TypeErrorhere
christabella commentedJan 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In what sense is C# 9 not ready? It would be great if this fix was merged, since that would unblock other issues e.g.#1040 |
lostmsu commentedJan 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@christabella@tminka if we bump version now, users of many older Mono builds will be unable to install pythonnet The problem is this branch relies on a new C# feature - unmanaged function pointers, which isbroken in the latest .NET Core SDK C# compiler. The fix is up in Roslyn's |
filmor commentedJan 13, 2021
@lostmsu The only C# 9 feature you are using here is |
lostmsu commentedJan 13, 2021
I expect older Mono versions to have old C# compiler, which simply won't recognize |
benoithudson commentedJan 13, 2021
I don't know what this is about exactly. For Unity, if we can target NET Standard 2.0 we're good. We don't use the Unity mcs to compile anyway, we ship this code as DLLs. Helpfully, Microsoft now tells you which version of Unity supports which standard. |
filmor commentedJan 13, 2021
We don't compile this project with the Mono C# compiler either, we expect a .NET Core SDK to be installed nowadays. This just bumps the required SDK version to 5.0, I think. |
filmor commentedJan 20, 2021
@lostmsu Since this passes tests, I'd vote we merge it but keep the bumped requirement in mind. |
lostmsu commentedJan 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@filmor heads up, MS just released VS 16.9 Preview 3 along with a new .NET SDK Preview, and the blocking issue I mentioned above appears to be fixed there. I should resume work on building a version independent DLL by the next sync. |
What does this implement/fix? Explain your changes.
Previously, when a floating point value was passed to .NET parameter of integer type, it would be silently truncated. After this change it would no longer be permitted to pass a floating point value to integer parameter.
Does this close any currently open issues?
#1342
Any other comments?
I removed poorly-behaved
PyLong_As*functions, as they would always implicitly convert argument to integer type, which in most cases is undesired behavior. Instead, to avoid roundtrips,PyLong_AsSize_tis used in most cases. The only exception isInt64which on 32 bit platforms is larger thansize_t, soPyLong_AsLongLongfunction is used after explicit check forPyLong.Draft because it depends on C# 9, which is not yet ready.
Checklist
Check all those that are applicable and complete.
CHANGELOG