Skip to content
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

floor div, variadic mod #1207

Merged
merged 3 commits into from Jul 1, 2023
Merged

floor div, variadic mod #1207

merged 3 commits into from Jul 1, 2023

Conversation

primo-ppcg
Copy link
Contributor

@primo-ppcg primo-ppcg commented Jun 30, 2023

Related issue: #1206

  • Adds floored division to corelib (div)
  • Makes mod and % variadic
  • Defines (mod x 0) as x, (% x 0) is unchanged

@sogaiu
Copy link
Contributor

sogaiu commented Jun 30, 2023

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
Copy link
Contributor Author

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.

See: Knuth, Donald E., _The Art of Computer Programming: Volume 1: Fundamental Algorithms_, pp. 15 ([link](https://books.google.com/books?id=x9AsAwAAQBAJ&pg=PA15))
@primo-ppcg primo-ppcg marked this pull request as ready for review June 30, 2023 14:03
@sogaiu
Copy link
Contributor

sogaiu commented Jul 1, 2023

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
Copy link
Contributor Author

primo-ppcg commented Jul 1, 2023

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:

janet/src/core/vm.c

Lines 731 to 736 in b125cbe

VM_OP(JOP_BNOT) {
Janet op = stack[E];
vm_assert_type(op, JANET_NUMBER);
stack[A] = janet_wrap_integer(~janet_unwrap_integer(op));
vm_pcnext();
}

I think that's all that would need to change, apart from the implementation in inttypes, of course.

Definitely a separate PR, though.

@bakpakin bakpakin merged commit db902c9 into janet-lang:master Jul 1, 2023
8 checks passed
@primo-ppcg primo-ppcg deleted the divmod branch July 1, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants