Update methodbinder.cs #217
Conversation
patch for pythonnet#203 - not best solution though.
| @@ -344,7 +344,7 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw) | |||
| if (clrtype != null) | |||
| { | |||
| bool typematch = false; | |||
| if (pi[n].ParameterType != clrtype) | |||
| if ((pi[n].ParameterType != clrtype) && !(pi[n].ParameterType.IsAssignableFrom(clrtype))) | |||
There was a problem hiding this comment.
Isn't the != check redundant? It looks like checking !pi[n].ParameterType.IsAssignableFrom(clrtype) would be sufficient.
There was a problem hiding this comment.
This is a very good catch! But actually the logic is more complicated,
since sometimes multiple methods can match IsAssignableFrom criteria for
the overloaded arguments.
So in addition to type equality really need to loop through IsSubclassOf
and build a list sorted by proximity to the argument type.
On Thu, May 26, 2016 at 4:43 AM, Tony Roberts notifications@github.com
wrote:
In src/runtime/methodbinder.cs
#217 (comment):@@ -344,7 +344,7 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw)
if (clrtype != null)
{
bool typematch = false;
if (pi[n].ParameterType != clrtype)if ((pi[n].ParameterType != clrtype) && !(pi[n].ParameterType.IsAssignableFrom(clrtype)))Isn't the != check redundant? It looks like checking
!pi[n].ParameterType.IsAssignableFrom(clrtype) would be sufficient.—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/pythonnet/pythonnet/pull/217/files/5a8ae4f6b1a69ad96c17d894972556296b7fe513#r64718246
There was a problem hiding this comment.
All I meant was that there can never be the case where pi[n].ParameterType == clrtype && !(pi[n].ParameterType.IsAssignableFrom(clrtype)), so this case is equivalent to checking that ParameterType is not assignable from clrtype.
Another way of saying this is that (ParameterType is not assignable from clrtype) implies (Parameter Type != clrtype). So, the expression as written is logically equivalent to simply (ParameterType is not assignable from clrtype).
|
Looks like appveyor is still being a rebel and not running. |
|
Unless I'm somehow downloading the wrong version again; this |
|
@denfromufa Should the |
|
I can custom merge or cherry pick the tests from your branch. But I'm still looking at how to resolve methods when multiple overloaded On Thursday, May 26, 2016, Vik notifications@github.com wrote:
|
|
Insufficient testing and no reply to comments for some time, so closing without merging. |
|
I came across this chapter and will look closer how to resolve ambiguous cases: |
|
@denfromufa did we ever come to a resolution for this? |
|
@vmuriart not yet, this change requires restructuring the way pythonnet searches the best match in overloaded methods. Right now there is assumption that only one match is possible and so pythonnet proceeds with the first match. However this is not how it works for C#. In short, we need to add a list with overloaded method matches and decide which one is the best. Otherwise, if more than one match is found, then throw an exception to let the user select the overloaded method precisely without any ambiguity in method resolution. |
#203
@vmuriart @tonyroberts