Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

@lostmsu
Copy link
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-behavedPyLong_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 isInt64 which on 32 bit platforms is larger thansize_t, soPyLong_AsLongLong function 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.

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

@lostmsulostmsuforce-pushed theno-implicit-float-to-int branch from7c4a6b5 toefe84c2CompareDecember 31, 2020 21:32
Copy link
Contributor

@tminkatminka left a 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
Copy link
Contributor

christabella commentedJan 7, 2021
edited
Loading

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

tminka reacted with thumbs up emoji

@lostmsu
Copy link
MemberAuthor

lostmsu commentedJan 7, 2021
edited
Loading

@christabella@tminka if we bump version now, users of many older Mono builds will be unable to install pythonnetmaster due to lack of C# 9 support. This will no longer be a problem after merginghttps://github.com/losttech/pythonnet/commits/features/VersionIndependent , which will allow building singlePython.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 isbroken in the latest .NET Core SDK C# compiler. The fix is up in Roslyn'smaster, but has not been released yet.

@filmor
Copy link
Member

@lostmsu The only C# 9 feature you are using here isnint, 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
Copy link
MemberAuthor

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

@benoithudson
Copy link
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
Copy link
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
Copy link
Member

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

@lostmsulostmsu marked this pull request as ready for reviewJanuary 20, 2021 22:36
@lostmsulostmsu merged commit0f33f71 intopythonnet:masterJan 20, 2021
@lostmsulostmsu deleted the no-implicit-float-to-int branchJanuary 20, 2021 22:36
@lostmsu
Copy link
MemberAuthor

lostmsu commentedJan 21, 2021
edited
Loading

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

filmor reacted with hooray emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@tminkatminkatminka left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lostmsu@christabella@filmor@benoithudson@tminka

[8]ページ先頭

©2009-2025 Movatter.jp