-
Notifications
You must be signed in to change notification settings - Fork 747
Improve method binding #974
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
Conversation
change enumerable method to non-static add tuple test for ienumerable
allow_threads in methodbinder.cs Invoke is true which is causing the memory corruption. If I set it to false there is no corruption so I need to look into why. This looks like it may be new code |
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.
I honestly think we'd be better off not introducing additional automatic conversions at this moment.
src/runtime/converter.cs
Outdated
Action action = () => { | ||
PyObject py_action = new PyObject(value); | ||
var py_args = new PyObject[0]; | ||
var py_result = py_action.Invoke(py_args); | ||
|
||
//Discard the result since this is being converted to an Action | ||
py_result.Dispose(); | ||
py_action.Dispose(); | ||
}; |
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.
The object pointed to by value
can be collected between the construction of this action
, and the moment it is invoked. It would lead to a memory corruption.
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.
I had forgotten to push the latest change set, please check again
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.
I honestly think we'd be better off not introducing additional automatic conversions at this moment.
That's an acceptable position. I'll bring this PR up at tomorrow's meeting and will like to hear from the group on how to proceed
Not suggesting this readonly implicit conversion, what if I want to use like this void AddElem(IList<int> lst)
{
lst.add(10);
} def func():
lst = [1, 2, 3]
AddElem(lst)
# I want [1,2,3,10] actually I suggest implement some subclasses from List<> so you can track the changes of the collection and apply them to the Python's list |
@amos402 see the latest changes. I'm dealing with some crash/exception in native code but I think I'm going in the direction that you are proposing. |
@lostmsu since we last spoke I looked into codec and it looks like string encoding, so I'm not sure how it could apply to method binding. What is your preferred way to support non-automatic conversions for me to consider/try? |
Runtime.XIncref(iterObject); | ||
} | ||
~IterableWrapper() { | ||
Runtime.XDecref(iterObject); |
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.
You can't use XDecref
directly in destructor, see Runtime.PyObject
's destructor, you can just refer a PyObject
, it can let you have no need concern about about the release.
Opened #1022 for my take on generic approach. |
What does this implement/fix? Explain your changes.
Two method binding changes:
Support passing lists/tuples to methods which take IEnumerable, ICollection, IList
Support passing functions to methods which take Action, Func, Task
Does this close any currently open issues?
Not that I am aware of.
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG