Skip to content

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