search

janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.45k stars 223 forks source link

floor div, variadic mod #1207

Closed primo-ppcg closed 1 year ago

primo-ppcg commented 1 year ago

Related issue: #1206

sogaiu commented 1 year ago

I wanted to bring up the issue of where tests related to int/*64 ought to live.

When working on the reorganization of tests recently, I made some changes in that direction, namely to place some things with int/s64 or int/u64 in suite-inttypes.janet. This was motivated by this comment having to do with (eventually?) arranging things so that we can:

run testing when certain features are disabled - for example, to be able to run make test and pass, even if FFI is disabled.

I don't think I got everything relevant moved in to suite-inttypes.janet so if this is a direction we want to go in, there are still some changes to be made.

Perhaps the comments above belong in a separate issue, but if there are to be tests in this PR that use int/*64 perhaps it would be worth thinking about where they should go. If it's not too much trouble may be they can go in suite-inttypes.janet?

primo-ppcg commented 1 year ago

I don't think I got everything relevant moved in to suite-inttypes.janet so if this is a direction we want to go in, there are still some changes to be made.

I'll be adding a few more tests to this suite. While I'm there, I can look into moving any remaining tests as well.

sogaiu commented 1 year ago

Thanks for the test moving / shielding in b3db367.

Looking over the proposed changes in 8a62c74, I was reminded of something @pyrmont mentioned recently about how int/64 and int/u64 do not provide complement / not. I didn't figure out whether there is some reason it isn't doable. Perhaps it's something that should have a separate issue.

primo-ppcg commented 1 year ago

I was reminded of something @pyrmont mentioned recently about how int/64 and int/u64 do not provide complement / not.

I can't immediately think of a reason why bnot shouldn't be supported by int types... although if I were to attempt to implement it, I suspect I may quickly discover the reason 😉

Firstly, the vm asserts that the type is :number rather than deferring to abstract implementations: https://github.com/janet-lang/janet/blob/b125cbeac9e43ca7adb3f066fef679ac58a91c71/src/core/vm.c#L731-L736 I think that's all that would need to change, apart from the implementation in inttypes, of course.

Definitely a separate PR, though.