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

Introduce BigInt type with toString method (no arithmetics yet) #175

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Rob23oba
Copy link
Contributor

@Rob23oba Rob23oba commented Aug 9, 2024

This pull request adds basic support for the BigInt type (bigint literals, toString). However, this pull request also contains unfinished changes to memory allocation so bugs might occur (specifically, static and dynamic allocation can collide).

[ Opcodes.i32_mul ]
wasm: (_, { builtin }) => [
...number(pageSize, Valtype.i32),
[16,builtin('__Porffor_allocateBytes')]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opcodes.call ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, don't know how that happened.


export const BigInt = function (value: number): any {
let result: BigInt = Porffor.wasm`
local.get ${value}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need to cast the argument to a number first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, again, it's incomplete currently.

runner/repl.js Outdated
@@ -117,6 +117,8 @@ const run = (source, _context, _filename, callback, run = true) => {

const replServer = repl.start({ prompt: '> ', eval: run });

replServer.on('reset', () => prev = "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's unrelated but also a good change.

// size
[ Opcodes.local_get, 2 ],

[ ...Opcodes.memory_copy, 0x00, 0x00 ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce __Porffor_copyMemory if it goes unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's supposed to be for memory allocation stuff and I kinda just accidentally built off of that.

@BobVarioa
Copy link
Contributor

Maybe I'm missing something as well, but are the memory changes really related to BigInts?

@Rob23oba
Copy link
Contributor Author

Yeah, I just had them in the same branch more or less accidentally.

@BobVarioa
Copy link
Contributor

Then, either this PR's title should be changed, or that functionality should be separated into another PR (the better choice imo)

@Rob23oba
Copy link
Contributor Author

Okay, this was slightly annoying to do but I now re-organized this branch to only contain bigint stuff

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.

2 participants