Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Conversation

amos402
Copy link
Member

@amos402 amos402 commented Jul 5, 2019

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 the MethodBinder. @lostmsu I realized we just did the same thing. But a little different was I intended to support the method like foo(int x = 0, params object[] bar) can be call with foo(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 to AUTHORS
  • Updated the CHANGELOG

Copy link
Member

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

{
if (tp == null)
{
yield break;
Copy link
Member

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 handle null case.

MethodInfo method;
try
{
method = t.MakeGenericMethod(tp);
Copy link
Member

Choose a reason for hiding this comment

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

You can yield return right here

}
catch (ArgumentException)
{
method = null;
Copy link
Member

Choose a reason for hiding this comment

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

You can continue right here

Type param = pi[n].ParameterType;

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

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 :(


public static void FixupSlots(IntPtr pyType, Type clrType)
{
IntPtr tp_as_number = Marshal.ReadIntPtr(pyType, TypeOffset.tp_as_number);
Copy link
Member

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
Member Author

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.

{
throw new PythonException();
}
OperatorMethod.FixupSlots(type, clrType);
Copy link
Member

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
Member Author

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

@lostmsu lostmsu Jul 6, 2019

Choose a reason for hiding this comment

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

@amos402 From Python 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 move FixupSlots up, and see if it works that way.

assert result[0] == 'one'
assert result[1] == 'two'
assert result[2] == 'three'
# Skip these temporally cause of the changes of array parameter calling
Copy link
Member

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 of params object[] arguments. If it was allowed previously, but is not after your change, it removes a feature, which should not be done.

return OpMethodMap.ContainsKey(methodName);
}

public static bool IsPyOperatorMethod(string pyMethodName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method seems to be unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was used later for delegates support? joonhwan@b154475

@tminka
Copy link
Contributor

tminka commented Dec 11, 2020

Fixes #1312

@christabella christabella mentioned this pull request Dec 16, 2020
4 tasks
@christabella
Copy link
Contributor

Could be closed in favour of the updated version at #1324

@lostmsu lostmsu closed this Jan 7, 2021
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