Skip to content

Constructor argument matching supports simple int to (float|double) types #239

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.

Sign up for GitHub

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

Closed
sigbjorn wants to merge 16 commits into from
Closed

Constructor argument matching supports simple int to (float|double) types #239

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2a2c9c7
added intial test that illustrates how int->double conversion works i…
sigbjorn Jun 30, 2016
78dca29
ease windows development, - default settings after checkout reflectin…
sigbjorn Jul 3, 2016
af1b937
added method tests for simple promotions int->float, int->double
sigbjorn Jul 3, 2016
24baadf
extending test to cover negative outcome of bool->double|float
sigbjorn Jul 3, 2016
bde4258
adding default ct to challenge the matching mechanism
sigbjorn Jul 3, 2016
c8c8d8c
merged extra tests
sigbjorn Jul 3, 2016
a65d793
adding test, and print-out warning for the not yet implemented feature
sigbjorn Jul 3, 2016
e698b57
added more test for illustrating super/sub-class and argument passing…
sigbjorn Jul 3, 2016
1fd75b3
fixed py 2.7 syntax for super, plus fixed typo
sigbjorn Jul 4, 2016
fb9b21e
reverted the .sln to unmodified state
sigbjorn Jul 4, 2016
9e1d72b
reverting changes to runtime and clrmodule projects
sigbjorn Jul 4, 2016
0e2cd12
Reverting changes to Console, embed_tests and testing .csproj files
sigbjorn Jul 4, 2016
e58d5b2
Hopefully fixed py 2.6 issues
sigbjorn Jul 4, 2016
8602823
Merge branch 'ct_arg_match_fix' of https://github.com/sigbjorn/python…
sigbjorn Jul 4, 2016
bdbaa62
SimplePromotableType simplified, returns true if src is int-type and …
sigbjorn Jul 23, 2016
478b632
Merge branch 'master' into ct_arg_match_fix
sigbjorn Jul 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
added intial test that illustrates how int->double conversion works i… 
…n calling a constructor, prior to the change in the methodbinder.cs, this test failed, since the no-match in ct. was failing silently, now it works. Next step is to give exception when ct args do not match
  • Loading branch information
@sigbjorn
sigbjorn committed Jun 30, 2016
commit 2a2c9c71ec0c6e54d95ca0a950dccbf07d323d1b
12 changes: 10 additions & 2 deletions pythonnet.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 2013
VisualStudioVersion = 12.0.30110.0
# Visual Studio 14
VisualStudioVersion = 14.0.25123.0
MinimumVisualStudioVersion = 10.0.40219.1
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Python.Runtime", "src\runtime\Python.Runtime.csproj", "{097B4AC0-74E9-4C58-BCF8-C69746EC8271}"
EndProject
Expand Down Expand Up @@ -89,10 +89,18 @@ Global
{E29DCF0A-5114-4A98-B1DD-71264B6EA349}.ReleaseWin|x64.Build.0 = Release|x64
{E29DCF0A-5114-4A98-B1DD-71264B6EA349}.ReleaseWin|x86.ActiveCfg = Release|x86
{E29DCF0A-5114-4A98-B1DD-71264B6EA349}.ReleaseWin|x86.Build.0 = Release|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugMono|x64.ActiveCfg = DebugMono|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugMono|x64.Build.0 = DebugMono|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugMono|x86.ActiveCfg = DebugMono|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugMono|x86.Build.0 = DebugMono|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugWin|x64.ActiveCfg = DebugWin|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugWin|x64.Build.0 = DebugWin|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugWin|x86.ActiveCfg = DebugWin|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.DebugWin|x86.Build.0 = DebugWin|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseMono|x64.ActiveCfg = ReleaseMono|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseMono|x64.Build.0 = ReleaseMono|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseMono|x86.ActiveCfg = ReleaseMono|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseMono|x86.Build.0 = ReleaseMono|x86
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseWin|x64.ActiveCfg = ReleaseWin|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseWin|x64.Build.0 = ReleaseWin|x64
{86E834DE-1139-4511-96CC-69636A56E7AC}.ReleaseWin|x86.ActiveCfg = ReleaseWin|x86
Expand Down
1 change: 1 addition & 0 deletions src/clrmodule/clrmodule.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
<CodeAnalysisIgnoreBuiltInRuleSets>true</CodeAnalysisIgnoreBuiltInRuleSets>
<CodeAnalysisIgnoreBuiltInRules>true</CodeAnalysisIgnoreBuiltInRules>
<CodeAnalysisFailOnMissingRules>false</CodeAnalysisFailOnMissingRules>
<DefineConstants>PYTHON34;UCS2</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Reference Include="RGiesecke.DllExport.Metadata">
Expand Down
3 changes: 2 additions & 1 deletion src/console/Console.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@
<CodeAnalysisIgnoreBuiltInRules>true</CodeAnalysisIgnoreBuiltInRules>
<CodeAnalysisFailOnMissingRules>false</CodeAnalysisFailOnMissingRules>
<WarningLevel>4</WarningLevel>
<DefineConstants>TRACE;PYTHON34, UCS2</DefineConstants>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'EmbeddingTest|x64'">
<DebugSymbols>True</DebugSymbols>
Expand Down Expand Up @@ -196,7 +197,7 @@
<ItemGroup>
<Content Include="python-clear.ico" />
<EmbeddedResource Include="$(PythonBuildDir)\Python.Runtime.dll">
<LogicalName>Python.Runtime.dll</LogicalName>
<LogicalName>Python.Runtime.dll</LogicalName>
</EmbeddedResource>
</ItemGroup>
<ItemGroup>
Expand Down
1 change: 1 addition & 0 deletions src/embed_tests/Python.EmbeddingTest.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
<CodeAnalysisIgnoreBuiltInRuleSets>true</CodeAnalysisIgnoreBuiltInRuleSets>
<CodeAnalysisIgnoreBuiltInRules>true</CodeAnalysisIgnoreBuiltInRules>
<CodeAnalysisFailOnMissingRules>false</CodeAnalysisFailOnMissingRules>
<DefineConstants>PYTHON34, UCS2</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Reference Include="System" />
Expand Down
1 change: 1 addition & 0 deletions src/runtime/Python.Runtime.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
<PlatformTarget>x64</PlatformTarget>
<CodeAnalysisIgnoreBuiltInRuleSets>false</CodeAnalysisIgnoreBuiltInRuleSets>
<CodeAnalysisIgnoreBuiltInRules>true</CodeAnalysisIgnoreBuiltInRules>
<DefineConstants>PYTHON34, UCS2</DefineConstants>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'DebugMono|x86'">
<DebugSymbols>true</DebugSymbols>
Expand Down
28 changes: 26 additions & 2 deletions src/runtime/methodbinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,20 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw,
Runtime.Decref(pyoptype);
if (!typematch)
{
margs = null;
break;
if (SimplePromotableType(clrtype, pi[n].ParameterType))
{
if (!Converter.ToManaged(op, pi[n].ParameterType, out arg, false))
{
margs = null;// basically we are failing this match, but can't throw yet
break;
}
}
else
{
margs = null;
break;
}

}
}
else
Expand Down Expand Up @@ -455,6 +467,18 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw,
}
return null;
}
/// <summary>
/// Returns true if src type (int -type) is promotable to a higher resolution type (float|double)
/// </summary>
/// <param name="src"></param>
/// <param name="dst"></param>
/// <returns></returns>
private bool SimplePromotableType(Type src, Type dst)
{
return (((src == typeof(int) || src == typeof(short)) && (dst == typeof(double) || dst == typeof(float))))
|| ((src == typeof(long)) && (dst == typeof(double)))
Copy link
Copy Markdown
Member

@filmor filmor Jul 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is a bit arbitrary, isn't it? A 64bit integer fits in neither double nor float, so why make this distinction here?

Copy link
Copy Markdown
Author

@sigbjorn sigbjorn Jul 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so it merely answers if its possible to do the conversion.
The real conversion is attempted later, using standard python.net conversion
that hopefully do the best effort, throws, etc. if there are numerical problems representing the one from the other.
We could even drop the test, and just do the python.net conversion, - that would kind of be consistent with what happen, I think, to usual function arguments. If that conversion fail, then it's not promotable.

The reason to be distinct on a numerical type is that we want to be as precise as possible.

And yes, - given that there is both and int and a double constructor, we should select the one with closest match. (Requires rewrite, or redirect logic to some function that already do this ?)

Copy link
Copy Markdown
Contributor

@den-run-ai den-run-ai Jul 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, - given that there is both and int and a double constructor, we should select the one with closest match. (Requires rewrite, or redirect logic to some function that already do this ?)

@sigbjorn as far as I'm aware there is no logic for resolution order in pythonnet for multiple matches due to complexity it brings into code-base:

#217

Copy link
Copy Markdown
Member

@filmor filmor Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that _dis_allowing the conversion long -> float doesn't make sense as the conversion long -> double is also lossy (64 vs 53). So either we allow all of those or none.

Copy link
Copy Markdown
Author

@sigbjorn sigbjorn Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, then we could try the approach I suggested in my initial comment, - just use the standard conversion function inside pythonnet. That's wider approach, but there is a hope it's consistently used for argument conversions, and thus we can terminate this discussion, since then the arguments to a constructor undergoes (almost) the same rules as an ordinary pythonnet function.
I am on vacation now, but can try to see if its possible to get in touch with a computer long enough to produce the change.

|| ((src ==typeof(int) || src == typeof(short)) && (dst == typeof(long)));
}

internal virtual IntPtr Invoke(IntPtr inst, IntPtr args, IntPtr kw)
{
Expand Down
1 change: 1 addition & 0 deletions src/testing/Python.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
<CodeAnalysisIgnoreBuiltInRuleSets>true</CodeAnalysisIgnoreBuiltInRuleSets>
<CodeAnalysisIgnoreBuiltInRules>true</CodeAnalysisIgnoreBuiltInRules>
<CodeAnalysisFailOnMissingRules>false</CodeAnalysisFailOnMissingRules>
<DefineConstants>PYTHON34, UCS2</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Compile Include="arraytest.cs" />
Expand Down
23 changes: 22 additions & 1 deletion src/testing/constructortests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,28 @@ namespace Python.Test
//========================================================================
// These classes support the CLR constructor unit tests.
//========================================================================

public class AConstrucorTest
Copy link
Copy Markdown
Member

@filmor filmor Jul 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo.

{
public string name;
public AConstrucorTest(string n) { name = n; }
}
public class LinkConstructorTest
{
public LinkConstructorTest()
{
DefaultCtCalled = true;
}
public LinkConstructorTest(AConstrucorTest a,double matchMe,AConstrucorTest b)
{
MatchMe = matchMe;
a1 = a;
a2 = b;
}
public bool DefaultCtCalled;
public double MatchMe;
public AConstrucorTest a1;
public AConstrucorTest a2;
}
public class EnumConstructorTest
{
public TypeCode value;
Expand Down
13 changes: 13 additions & 0 deletions src/tests/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,19 @@ class sub(System.Exception):
print(ob)
self.assertTrue(isinstance(ob.value, System.Exception))

def testConstructorArgumentMatching(self):
""" Test that simple type promitions works for int->double """
from Python.Test import AConstrucorTest, LinkConstructorTest
a1=AConstrucorTest('a1')
a2=AConstrucorTest('a2')
self.assertEqual(a1.name,'a1')
self.assertEqual(a2.name, 'a2')
l1=LinkConstructorTest(a1,3000,a2)
#l2=LinkConstructorTest(a1,3000.0,a2)
self.assertEqual(l1.a1.name, a1.name)
self.assertEqual(l1.a2.name, a2.name)
self.assertAlmostEqual(3000.0,l1.MatchMe)


def test_suite():
return unittest.makeSuite(ConstructorTests)
Expand Down