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

Fixed string concat to cstring conversion #427

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

wnats
Copy link
Collaborator

@wnats wnats commented Dec 8, 2023

?str = ("0" ,, "1") ,, "2"

!println(c_string(str))

would incorrectly output "01", at least in my local machine which at that time happened to allocate a zeroed out memory chunk on an lpvm alloc. This problem is more noticeable when calling !logmsg(<log_str>), as <log_str> is first converted into a cstring.

The culprit seems to be the incr(!offset) line. The line is unnecessary as offset has been incremented accordingly in each recursive pack call.

| concat(?left, ?right) ::
pack(left, !raw, size, !offset)
pack(right, !raw, size, !offset)
incr(!offset)

Perhaps unrelated to the main problem, but I have also changed the third argument of mutate to len - 1, which I believe is more accurate, although I have yet to see a corner case for this.

pub def c_string(s:_):c_string = str where {
if { s = buffer(_, ?str) :: pass
| else ::
?len = length(s) + 1
foreign lpvm alloc(len, ?str)
foreign lpvm mutate(str, ?str, len, true, len, 0, '\0')
?offset = 0
pack(s, !str, len, !offset)
}
}

@wnats wnats added the bug Something isn't working label Dec 8, 2023
@wnats wnats requested review from pschachte and jimbxb December 8, 2023 05:45
@wnats wnats self-assigned this Dec 8, 2023
@jimbxb
Copy link
Collaborator

jimbxb commented Dec 8, 2023

LGTM. This is a good catch!

Copy link
Owner

@pschachte pschachte left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

@pschachte pschachte merged commit 63c971d into pschachte:master Dec 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants