-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Float Conversion for #211 #287
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
vm/src/stdlib/math.rs
Outdated
| let value = objfloat::get_value(value).is_infinite(); | ||
| if args.args.len() != 1 { | ||
| return Err(vm.new_type_error(format!( | ||
| "erf() takes exactly one argument ({} given)", |
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 error string contains the wrong function name erf. Maybe it makes sense to create a seperate function for this double extraction. The code is now copy pasted about 3 times. Please create a helper function which checks the amount of arguments and extracts the floating point number.
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 completely agree with you that this copy-paste is horrible solution. I hoped to start a discussion from it.
I think this helper should be integrated into arg_check macro.
Whereve arguments are parsed and one of them is expected to be float it can actually be any other real number type macro should really check make_float, otherwise it'll be cpython incompatible
In [1]: class A:
...: def __float__(self):
...: return 1.0
...:
In [2]: float(A())
Out[2]: 1.0
In [3]: import math
In [4]: math.log(A())
Out[4]: 0.0
So I hope you can give an advice on this one. what'd be the best way to aproach it?
Should it be just hardcoded into a macro as a special case?
I'm also pretty sure that the same implicit conversion should be done for for int
For example in this case I'm passing True as a port number for socket and it only fails, because of premissions, (it works from root!).
In [1]: import socket
In [3]: s = socket.socket()
In [4]: s.bind(('localhost', True))
---------------------------------------------------------------------------
PermissionError Traceback (most recent call last)
<ipython-input-4-ced4611914b1> in <module>()
----> 1 s.bind(('localhost', True))
PermissionError: [Errno 13] Permission denied
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 would not yet place this automatic coercion in the arg_check macro yet. This macro is used really a lot. I suggest to create a helper function like coerce_into_float or something like that and place that in objfloat.rs for now. We can always extend the arg_check macro later when we require this logic in all places.
Basically my doubt is: Are you sure we always need to convert int to float at all function calls?
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.
Ok, fair point.
One more question then, if you don't mind:
Can arg_check be used to check just a number of arguments, without checking a type?
It'll be handy to use arg_check to control a number of arguments a new helper function, to control their types.
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.
Yes, this is possible. I believe you can set the type to None, and then it will extract the correct amount of arguments into the given rust names.
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.
That's great. I'll try to come back with a fix as soon as possible
windelbouwman
left a comment
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.
Please create a helper function to extract the proper arguments as mentioned in another comment.
e1e2120 to
b25aab0
Compare
|
Sorry, I'm not sure it's ok here, but I have a habbit of force-pushing development branches occasionally, so I did this. Anyway. It went ok without much code duplication without helper method, since I'm using (as you've adviced) I've also added python based test for this. |
|
I see that Travis build failed with Which I don't really understand. It might be platform-dependent |
Assuming `exp` function was missing for platform-dependant reasons, let's try to test it with `sin` instead
…to float-conversion
Implemented float conversion for functions in
mathmodule