Skip to content

Add rem builtin for i32 and i64 (#1310) #2306

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

Merged
merged 4 commits into from
Jun 10, 2022

Conversation

adrien-zinger
Copy link
Contributor

@adrien-zinger adrien-zinger commented Jun 3, 2022

Implement the builtin rem for i32 and i64, requested in the issue #1310

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@MaxGraey
Copy link
Member

MaxGraey commented Jun 3, 2022

Hi! Thanks for PR. Similar to division builtins it should be splitted to rem_s and rem_u. See:

function builtin_i32_div_s(ctx: BuiltinContext): ExpressionRef {

function builtin_i32_div_u(ctx: BuiltinContext): ExpressionRef {

@adrien-zinger
Copy link
Contributor Author

Thanks for the review @MaxGraey! I just implemented the split. I have a doubt on types in the std because the div_s and div_u have both a i32/64 type for the parameters. It should be u32/64 right? Even if the compiler will reinterpret those types later (if I understand correctly 😄 ).

@MaxGraey MaxGraey changed the title Add rem builtin for i33 and i64 (#1310) Add rem builtin for i32 and i64 (#1310) Jun 5, 2022
@MaxGraey MaxGraey requested a review from dcodeIO June 5, 2022 05:27
@dcodeIO
Copy link
Member

dcodeIO commented Jun 5, 2022

Thanks, looks good, just a few nits on wording above :)

Regarding

I have a doubt on types in the std because the div_s and div_u have both a i32/64 type for the parameters. It should be u32/64 right? Even if the compiler will reinterpret those types later (if I understand correctly 😄 ).

In the .d.ts, the numeric types are merely aliases of number, so I guess that's why the discrepancy hasn't been noticed before. Doesn't harm to unify these, as you suggest. Perhaps as another PR?

@adrien-zinger
Copy link
Contributor Author

In the .d.ts, the numeric types are merely aliases of number, so I guess that's why the discrepancy hasn't been noticed before. Doesn't harm to unify these, as you suggest. Perhaps as another PR?

Thanks for your review! I can change that in another PR as you said 👍

@adrien-zinger adrien-zinger requested a review from dcodeIO June 8, 2022 06:59
@dcodeIO dcodeIO merged commit 67457f3 into AssemblyScript:main Jun 10, 2022
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.

3 participants