- Notifications
You must be signed in to change notification settings - Fork749
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
change enumerable method to non-staticadd tuple test for ienumerable
koubaa commentedDec 1, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 byvalue can be collected between the construction of thisaction, 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
Uh oh!
There was an error while loading.Please reload this page.
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 voidAddElem(IList<int>lst){lst.add(10);} deffunc():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 useXDecref directly in destructor, seeRuntime.PyObject's destructor, you can just refer aPyObject, it can let you have no need concern about about the release.
Opened#1022 for my take on generic approach. |
Uh oh!
There was an error while loading.Please reload this page.
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.
AUTHORSCHANGELOG