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

Use smaller buffer for native int to str coercion #1272

Conversation

MasterDuke17
Copy link
Contributor

There can only be up to 20 chars, and the actual worker functions only
use 20 char arrays also.

NQP builds ok and passes make m-test and Rakudo builds ok and passes make m-test m-spectest.

There can only be up to 20 chars, and the actual worker functions only
use 20 char arrays also.
@niner
Copy link
Contributor

niner commented Apr 17, 2020

I think there's an off-by-one here as the buffer gets ended with a null byte: *buffer = '\0'; which I think is actually unnecessary.

@MasterDuke17
Copy link
Contributor Author

I think there's an off-by-one here as the buffer gets ended with a null byte: *buffer = '\0'; which I think is actually unnecessary.

That obviously pre-exists my change, but I'm seeing what happens if it's removed right now.

@MasterDuke17
Copy link
Contributor Author

NQP and Rakudo and all tests are ok with removing it. Should I add that to this PR or create a new one?

@niner
Copy link
Contributor

niner commented Apr 17, 2020

Just add it please

MoarVM doesn't use null-terminated strings, so this is unneeded.
@MasterDuke17 MasterDuke17 force-pushed the use_smaller_buffer_for_native_int_to_str_coercion branch from 27ee006 to bb27aeb Compare April 17, 2020 13:28
@MasterDuke17
Copy link
Contributor Author

Just add it please

Done.

@niner niner merged commit 57592e6 into MoarVM:master Apr 17, 2020
@MasterDuke17 MasterDuke17 deleted the use_smaller_buffer_for_native_int_to_str_coercion branch April 17, 2020 15:34
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

2 participants