Update methodbinder.cs #137
Conversation
|
@tonyroberts tests FAILED (errors=36), any idea how to fix this? |
|
@tonyroberts please let me know if you can merge this PR? I still need to write some tests for cases described in this issue #131. |
| if (pyoptype != IntPtr.Zero) { } | ||
| type = Converter.GetTypeByAlias(pyoptype); | ||
| } | ||
| //Runtime.Decref(pyoptype); |
There was a problem hiding this comment.
Is pyoptype being leaked here? PyObject_Type returns a new reference.
There was a problem hiding this comment.
@tonyroberts I'm not sure here, I actually wanted to ask you how to clean this up, especially if there is exception thrown? That's why I have Runtime.Decref(pyoptype) commented out :)
| //try { | ||
| pyoptype = Runtime.PyObject_Type(op); | ||
| /*} | ||
| catch (System.Exception) { |
There was a problem hiding this comment.
Please don't leave commented code - you commented it out because it was incorrect; it should be deleted instead.
There was a problem hiding this comment.
deleted
On Tue, Feb 9, 2016 at 1:10 PM, Tony Roberts notifications@github.com
wrote:
In src/runtime/methodbinder.cs
#137 (comment):op = Runtime.PyTuple_GetItem(args, n); }
Type type = pi[n].ParameterType;type = null;if (_methods.Length>1) {IntPtr pyoptype = IntPtr.Zero;//try {pyoptype = Runtime.PyObject_Type(op);/*}catch (System.Exception) {Please don't leave commented code - you commented it out because it was
incorrect; it should be deleted instead.—
Reply to this email directly or view it on GitHub
https://github.com/pythonnet/pythonnet/pull/137/files#r52357482.
|
Hi @denfromufa, I've just left a couple more comments. Once you've had a chance to take a look at those you should squash these commits together into a single commit (see git rebase -i). thanks, |
|
@tonyroberts I use web interface in github without git - so here I'm using the option "Add more commits by pushing to the patch-2 branch on denfromufa/pythonnet": https://github.com/denfromufa/pythonnet/tree/patch-2 I have a different source control locally. |
|
I've rebased the commits and updated the commit comment to something more meaningful. See PR #151. I don't really have the time to fix up your pull requests, so please take more care in the future and learn how to use git rebase. |
this fixes issue #131
I tested on System.Math.Abs(50.5) & System.Math.Max(50.5,50.1)