NotificationsYou must be signed in to change notification settings
Fork768
Star5.4k

Basic operator overloads support#907

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

Jump to bottom

Closed

amos402 wants to merge4 commits intopythonnet:masterfromamos402:operator

Closed

Basic operator overloads support#907

amos402 wants to merge4 commits intopythonnet:masterfromamos402:operator

Conversation

Copy link

Member

amos402 commentedJul 5, 2019•
edited
Loading

What does this implement/fix? Explain your changes.

Support the operator for objects.

I found this code from my branch before, not only support the operator but also refactored theMethodBinder.@lostmsu I realized we just did the same thing. But a little different was I intended to support the method likefoo(int x = 0, params object[] bar) can be call withfoo(1,1,1), you can see the change from the pytest. I think we need to discuss how to merge them.
I develop it base on Python3, some test cases may fail on 2 now.

Does this close any currently open issues?

#906

Any other comments?

...

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
Add yourself toAUTHORS
Updated theCHANGELOG

amos402 added2 commits

July 6, 2019 00:08

Basic operator overloads support

799bdae

Tests forfoo(int x = 0, params object[] bar)

e483fc9

amos402 mentioned this pull request

Jul 5, 2019

CLR operator and Python operator are not bound#906

Closed

lostmsu requested changes

Jul 6, 2019

View reviewed changes

Copy link

Member

lostmsu left a comment

Choose a reason for hiding this comment

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

Completed initial pass with some comments.

src/runtime/methodbinder.csShow resolvedHide resolved

src/runtime/methodbinder.cs

		{
		if(tp==null)
		{
		yieldbreak;

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

I think this should throw instead, and the caller should explicitly handlenull case.

src/runtime/methodbinder.cs

		MethodInfomethod;
		try
		{
		method=t.MakeGenericMethod(tp);

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

You canyield return right here

src/runtime/methodbinder.cs

		}
		catch(ArgumentException)
		{
		method=null;

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

You cancontinue right here

src/runtime/methodbinder.cs

		Typeparam=pi[n].ParameterType;

		if(!param.IsGenericParameter&&!IsNullableOf(sig,param)&&
		!param.IsAssignableFrom(sig))

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

Can you explain what this method was supposed to be doing, and what your change enables it to do now? It is actually not clear what genericTp and sigTp represent from their names :(

src/runtime/operator.cs


		publicstaticvoidFixupSlots(IntPtrpyType,TypeclrType)
		{
		IntPtrtp_as_number=Marshal.ReadIntPtr(pyType,TypeOffset.tp_as_number);

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

This does not seem to be used anywhere

Copy link

MemberAuthor

amos402Jul 6, 2019

Choose a reason for hiding this comment

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

😂opps, some methods are just used in my code, so it maybe not used here.

src/runtime/pythonengine.csShow resolvedHide resolved

src/runtime/typemanager.cs

		{
		thrownewPythonException();
		}
		OperatorMethod.FixupSlots(type,clrType);

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

Should not the fixup happen before PyType_Ready?

Copy link

MemberAuthor

amos402Jul 6, 2019

Choose a reason for hiding this comment

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

It could be.

Copy link

Member

lostmsuJul 6, 2019•
edited
Loading

Choose a reason for hiding this comment

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

@amos402 FromPython doc:

Finalize a type object. This should be called on all type objects to finish their initialization. This function is responsible for adding inherited slots from a type’s base class. Return 0 on success, or return -1 and sets an exception on error.

Unfortunately, the description does not clarify if the behavior could change in the future :/ I'd try to moveFixupSlots up, and see if it works that way.

src/tests/test_array.pyShow resolvedHide resolved

src/tests/test_method.py

		assertresult[0]=='one'
		assertresult[1]=='two'
		assertresult[2]=='three'
		# Skip these temporally cause of the changes of array parameter calling

Copy link

Member

lostmsuJul 6, 2019

Choose a reason for hiding this comment

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

Is this change not ready?
.NET permits arrays to be passed in place ofparams object[] arguments. If it was allowed previously, but is not after your change, it removes a feature, which should not be done.