FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Disable implicit conversion from PyFloat to .NET integer types by lostmsu · Pull Request #1343 · pythonnet/pythonnet · GitHub

Disable implicit conversion from PyFloat to .NET integer types - #1343

Merged
lostmsu merged 1 commit into
pythonnet:masterfrom
losttech:no-implicit-float-to-int
Jan 20, 2021
Merged

Disable implicit conversion from PyFloat to .NET integer types#1343
lostmsu merged 1 commit into
pythonnet:masterfrom
losttech:no-implicit-float-to-int

Conversation

lostmsu commented Dec 31, 2020

Copy link
Copy Markdown
Member

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_t is used in most cases. The only exception is Int64 which on 32 bit platforms is larger than size_t, so PyLong_AsLongLong function is used after explicit check for PyLong.

Draft because it depends on C# 9, which is not yet ready.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Updated the CHANGELOG

lostmsu force-pushed the no-implicit-float-to-int branch from 7c4a6b5 to efe84c2 Compare December 31, 2020 21:32

tminka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

test_uint64_conversion should similarly raise a TypeError here

christabella commented Jan 7, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jan 7, 2021
edited
Loading

Copy link
Copy Markdown
Member Author

@christabella @tminka if we bump version now, users of many older Mono builds will be unable to install pythonnet master due to lack of C# 9 support. This will no longer be a problem after merging https://github.com/losttech/pythonnet/commits/features/VersionIndependent , which will allow building single Python.Runtime.dll for all Python versions (so they don't have to build it).

The problem is this branch relies on a new C# feature - unmanaged function pointers, which is broken in the latest .NET Core SDK C# compiler. The fix is up in Roslyn's master, but has not been released yet.

filmor commented Jan 13, 2021

Copy link
Copy Markdown
Member

@lostmsu The only C# 9 feature you are using here is nint, isn't it? As far as I understood this, the feature doesn't require help from the CLR, so it should work with older Mono versions as well. @BadSingleton @benoithudson Could you verify that this branch still works with the Mono version you are using?

lostmsu commented Jan 13, 2021

Copy link
Copy Markdown
Member Author

I expect older Mono versions to have old C# compiler, which simply won't recognize nint.

Copy link
Copy Markdown
Contributor

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.
https://docs.microsoft.com/en-us/dotnet/standard/net-standard

filmor commented Jan 13, 2021

Copy link
Copy Markdown
Member

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 commented Jan 20, 2021

Copy link
Copy Markdown
Member

@lostmsu Since this passes tests, I'd vote we merge it but keep the bumped requirement in mind.

lostmsu marked this pull request as ready for review January 20, 2021 22:36
lostmsu merged commit 0f33f71 into pythonnet:master Jan 20, 2021
lostmsu deleted the no-implicit-float-to-int branch January 20, 2021 22:36

lostmsu commented Jan 21, 2021
edited
Loading

Copy link
Copy Markdown
Member Author

@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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

5 participants


Back | FazBrowse Home | New Git URL