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

Feat: Buffer api #731

Merged
merged 24 commits into from
Jan 30, 2024
Merged

Feat: Buffer api #731

merged 24 commits into from
Jan 30, 2024

Conversation

liz3
Copy link
Contributor

@liz3 liz3 commented Jan 23, 2024

Buffer API

What's Changed:

Implements a Buffer API inspired by Node.js's buffer api, including writing strings and all integer types.
Sadly even though i tried to reduce it as far as i could theres a loooot of duplication because of the fact there are 4 integer types, im open to suggestions which can help reduce the duplication.

Note: I commited .clang-format because thats the formatter i use and it's default setting is set to 2 spaces and so i thought it might be useful, can remove if not needed

Type of Change:

  • Bug fix
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Housekeeping:

  • Tests have been updated to reflect the changes done within this PR (if applicable).
  • Documentation has been updated to reflect the changes done within this PR (if applicable).

Screenshots (If Applicable):

image

@briandowns
Copy link
Contributor

This is super cool. Reminds me that we still need to get that native bytes type implemented.

As for the PR, I'll refrain from commenting until you're ready but I'll mention there's a few places that still have the original code / names from the copy/paste. I do the exact same thing. :)

Side note, and happy to take to a different forum like the discussion section but would love to understand your uses of Dictu!

@liz3
Copy link
Contributor Author

liz3 commented Jan 23, 2024

I wonder mostly if theres a way to reduce the copy paste for the integer methods which currently take up the majority of the file and only contain very small differences, thinking to have a generalised read/write to reduce it and then just have the typecast to the correct integer type.

Regarding forum you can mail me: [email protected] or discord: liz3

@Jason2605
Copy link
Member

Damn yeah this looks really neat! What I'll do with it being WIP is mark it as draft, then just mark it as ready whenever you're good to go!

@Jason2605 Jason2605 marked this pull request as draft January 24, 2024 20:11
@liz3 liz3 changed the title Feat: Buffer api(WIP) Feat: Buffer api Jan 25, 2024
@liz3 liz3 marked this pull request as ready for review January 25, 2024 20:53
@Jason2605
Copy link
Member

Sorry for being slow on this! @briandowns be good to get your thoughts on this one too if you get a spare moment!

Copy link
Contributor

@briandowns briandowns left a comment

Choose a reason for hiding this comment

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

This is such an awesome addition. Only a few small comments. Only other thing, which is a small nit pick, would be to add new line chars to the files that are missing them.

I'm already getting some ideas on what I can do with this!

tests/buffer/get.du Outdated Show resolved Hide resolved
docs/docs/standard-lib/buffer.md Outdated Show resolved Hide resolved
docs/docs/standard-lib/buffer.md Outdated Show resolved Hide resolved
docs/docs/standard-lib/buffer.md Show resolved Hide resolved
docs/docs/standard-lib/buffer.md Outdated Show resolved Hide resolved
Copy link
Contributor

@briandowns briandowns left a comment

Choose a reason for hiding this comment

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

LGTM! Excited to get this in. :D

Copy link
Member

@Jason2605 Jason2605 left a comment

Choose a reason for hiding this comment

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

Couple of pretty minor points, thanks so much for the effort on this!

Comment on lines 27 to 33
void grayBuffer(DictuVM *vm, ObjAbstract *abstract) {
(void)vm;
Buffer *ffi = (Buffer *)abstract->data;

if (ffi == NULL)
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed as we aren't holding any objects that could get GC'd within the buffer


void freeBuffer(DictuVM *vm, ObjAbstract *abstract) {
Buffer *buffer = (Buffer *)abstract->data;
free(buffer->bytes);
Copy link
Member

Choose a reason for hiding this comment

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

We want to try to use the macro FREE / ALLOCATE where possible, in theory it means in the future if we ever want to swap out the memory allocation methods it should be a single place swap

vm, "size must be greater than 0 and smaller than 2147483647");
}
Buffer *buffer = AS_BUFFER(args[0]);
buffer->bytes = realloc(buffer->bytes, capacity);
Copy link
Member

Choose a reason for hiding this comment

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

Same here with ALLOCATE


Buffer *buffer = ALLOCATE(vm, Buffer, 1);
buffer->bigEndian = false;
buffer->bytes = calloc(1, capacity);
Copy link
Member

Choose a reason for hiding this comment

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

same here

@liz3
Copy link
Contributor Author

liz3 commented Jan 29, 2024

Couple of pretty minor points, thanks so much for the effort on this!

Should be addressed.

@liz3
Copy link
Contributor Author

liz3 commented Jan 30, 2024

Why do the tests for *nix likes take 2 hrs? Is this coming from my changes or githubs runners?

@briandowns
Copy link
Contributor

My guess is that it's the runners. On Linux, MacOS, and FreeBSD, (locally) the suite runs very quickly.

@Jason2605
Copy link
Member

Thanks so much for this!

@Jason2605 Jason2605 merged commit aef7bc6 into dictu-lang:develop Jan 30, 2024
8 checks passed
@Jason2605 Jason2605 mentioned this pull request Sep 12, 2024
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