kyren / piccolo

An experimental stackless Lua VM implemented in pure Rust
Creative Commons Zero v1.0 Universal
1.68k stars 63 forks source link

Added other math functions (except random) #11

Closed OmnipotentEntity closed 5 years ago

OmnipotentEntity commented 5 years ago

and cleaned up argument handling (now abs(2, 3) is an error). This also includes some minor bug fixes such as floor and ceil returning a Number instead of an Integer.

I'm somewhat concerned about how max and min are handled. It feels very complicated for what is essentially a fold. I might be missing some way of properly handling this sort of thing. I couldn't make the ? operator work like I was expecting.

Tests to come soon (TM)

kyren commented 5 years ago

I don't think there are any Lua stdlib functions that check for extra arguments.

> math.acos(0.0, "foo")
1.5707963267949
kyren commented 5 years ago

Regarding min / max, you can either not use a fold at all, or you can use Iterator::try_fold.

kyren commented 5 years ago

Also behavior of min / max with no arguments:

> math.min()
stdin:1: bad argument #1 to 'min' (value expected)
kyren commented 5 years ago

Just curious, why rand_xoshiro specifically?

kyren commented 5 years ago

Okay, something I'm going to be working on soon is changing both the argument and return types to callbacks to avoid creating intermediate buffers. There will be a type that callbacks can't create that serves as both the argument and return vector, with operations to move args / returns around in the vector, and callbacks will be required to return it.

This is going to be an involved refactor or every callback, would you like me to make this change before or after merging this PR?

OmnipotentEntity commented 5 years ago

rand_xoshiro was chosen because it is very fast and has decent rng properties. It is not unique by any stretch of the imagination, it's just the built in random function I thought was best when I glanced once briefly over the built-in engines in the rand crate documentation.

OmnipotentEntity commented 5 years ago

This is going to be an involved refactor or every callback, would you like me to make this change before or after merging this PR?

Either way is fine, I'm OK with changing the callbacks to a new convention myself, or you can at your option. The only thing that's keeping this PR from being complete is tests for functions alphabetically beyond frexp. And if I'm keeping you from working on something that needs to get done, just do it, and I'll catch up. This code isn't the most complicated in the world. :)

OmnipotentEntity commented 5 years ago

Hopefully, this ought to be the rest of the needed tests. There are a few edge cases that I'm aware of where behavior of the functions does not line up 1 to 1 with lua, such as the min function does not handle nan cases in quite the same way. And some functions will accept strings with numbers in them where lua just barfs on it (such as the max and min functions), I'm not sure if you want to adhere to the behavior of PUC Lua on this because the documentation and specification do not say that this should be either allowed or disallowed. (Moreover, the behavior of PUC Lua also does not line up to the documentation either in the case of mixed integer/float types, max(1, 1.0, 1) gives Integer(1) rather than Float(1) as the documentation would suggest.)

kyren commented 5 years ago

I think it's important that all tests pass both luster and PUC-Rio Lua, so if there are any tests that aren't like that we shouldn't codify the current mismatched behavior of luster via the tests. It would even be nice if eventually rlua was used to verify this.

It's fine if it's not perfectly compatible right now in the interests of simple implementations, because there are bigger compatibility fish to fry first, but if you want to leave commented out tests that don't match, that would be excellent.

OmnipotentEntity commented 5 years ago

All test cases work in both luster and PUC-Rio Lua. I'll add some tests and comment them out where the behavior is mismatched.

kyren commented 5 years ago

I lied about refactoring arguments / returns, I can't come up with a sane API atm and I'm stalled working on it.

I'm going to go ahead and merge this PR.