-
Notifications
You must be signed in to change notification settings - Fork 30
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
Workaround for Math variadic functions #82
base: main
Are you sure you want to change the base?
Workaround for Math variadic functions #82
Conversation
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.
Might it be better to just remove the @variadic? (edit: sorry, didn't mean to include this)
src/Core__Math.res
Outdated
@variadic @val external _minMany: array<float> => float = "Math.min" | ||
let minMany = arr => | ||
switch arr { | ||
| [] => 0.0 | ||
| arr => _minMany(arr) | ||
} |
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'm not sure the float
versions ought to be overridden. Math.min([])
does indeed return 0
, and Math.min()
returns Infinity
, but Math.minMany([1, 2, 3])
maps to Math.min(1, 2, 3)
, not Math.min([1, 2, 3])
, and so it seems more consistent to have Math.minMany([])
map to Math.min()
, not Math.min([])
.
These are also edge cases that are very unlikely to occur in practice, since variadic calls require syntactic arrays there's little chance of it happening by accident. I'd suggest the aim of this is restricted to just making the types sound.
If you remove |
Ah, sorry, I didn't mean to include that. I checked after writing it, but apparently forgot to remove the comment. Please see the other comments instead :) |
I was wondering and not sure if we should return |
This ensures that the return for an empty array does not return
-Infinity
TODO:
See #53