- Notifications
You must be signed in to change notification settings - Fork750
Update methodbinder.cs#217
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
patch forpythonnet#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't the != check redundant? It looks like checking !pi[n].ParameterType.IsAssignableFrom(clrtype) would be sufficient.
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.
This is a very good catch! But actually the logic is more complicated,
since sometimes multiple methods can matchIsAssignableFrom criteria for
the overloaded arguments.
So in addition to type equality really need to loop throughIsSubclassOf
and build a list sorted by proximity to the argument type.
On Thu, May 26, 2016 at 4:43 AM, Tony Robertsnotifications@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, Viknotifications@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