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

Allocate rbs_string_t with allocator #14

Merged
merged 2 commits into from
Mar 12, 2025
Merged

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Mar 11, 2025

  1. rbs_string_t owned by the parser is now allocated by the allocator
  2. Because of 1, we don't need to care if a string is owned or shared anymore, so a lot of associated code can now go away

@st0012 st0012 requested review from Morriar and amomchilov March 11, 2025 21:41
@st0012 st0012 force-pushed the allocate-string-with-allocator branch from 1fff656 to f0467a1 Compare March 11, 2025 21:46
Copy link

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines 9 to 11
return (rbs_string_t) {
.start = start,
.end = end,

Choose a reason for hiding this comment

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

So if I understand correctly, this is still a "shared"/"unowned" string, it just doesn't need to be tagged as such, because we're never going to free() any RBS strings anyway.

We might still want the shared/owned distinction for debugging, but this makes sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes a string will be either "owned by the allocator" or "owned by something else" but we don't have to distinguish them anymore as the freeing is not manual anymore.

I prefer not to leave code around just for potential debugging purpose tho.

rbs_string_t rbs_string_copy_slice(rbs_string_t *self, size_t start_inset, size_t length) {
char *buffer = (char *) malloc(length + 1);
rbs_string_t rbs_string_copy_slice(rbs_allocator_t *allocator, rbs_string_t *self, size_t start_inset, size_t length) {
char *buffer = rbs_allocator_calloc(allocator, length + 1, char);

Choose a reason for hiding this comment

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

Follow-up: we're seeing a few of their cases where calloc is convenient for its API, but we don't need its behaviour memset(0, ...) (since we immediately overwrite it, here).

Perhaps we can add something like rbs_allocator_alloc_many which takes a count like calloc doesn, but doesn't 0-out all the memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will implement right after the PR is merged 👍

@st0012 st0012 force-pushed the allocate-string-with-allocator branch from f0467a1 to 00945c7 Compare March 12, 2025 13:37
Since we don't manually allocate/free strings anymore, we don't need the string type enum
and all the complexity that comes with it.
@st0012 st0012 force-pushed the allocate-string-with-allocator branch from 00945c7 to ee16286 Compare March 12, 2025 13:51
@st0012 st0012 merged commit 2e28e02 into c-api Mar 12, 2025
19 checks passed
@st0012 st0012 deleted the allocate-string-with-allocator branch March 12, 2025 13:59
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