Skip to content

Store contents of static string struct in the array buffer #42

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

Merged
merged 7 commits into from
Feb 28, 2023

Conversation

ngoldbaum
Copy link
Member

This is a followup for #41, since I had to make a number of extensive changes relative to what that PR originally proposed, I issued a new PR instead to hopefully make this clearer.

Instead of using flexible-length arrays, the ss struct now looks like this:

typedef struct ss {
    size_t len;
    char *buf;
} ss;

Rather than store a pointer to an ss struct in the array buffer, this refactors things so we instead store the contents of the ss struct in the array buffer. This reduces the amount of pointer indirection we need to do to get access to the ss struct data and also makes it much easier to deal with empty strings, since with this new definition ((ss*)NULL)->len is zero by construction.

Instead of mallocing storage for the entire struct on the heap, we now only malloc space for the string buffer, and the caller of functions that allocate memory are responsible for handling memory for the ss struct.

To handle this change, I had to rewrite the static_string API to accept pointers to ss instances instead of returning them. I also added a new load_string helper that returns either a read-only view onto an ss struct, or if the struct is NULL, returns a view onto a statically allocated empty struct.

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Didn't have a thorough look through, but I like this, thanks.

Comment on lines 28 to 32
// Allocates a new string buffer for *out* with enough capacity to store
// *num_bytes* of text. The actual allocation is num_bytes + 1 bytes, to
// account for the null terminator. Does not do any initialization, the caller
// must initialize and null-terminate the string buffer. Errors if *out*
// already contains data or if malloc fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀 This is super valuable documentation. Thank you!

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Reducing the number of stars everywhere makes this so much more readable, thanks for this! ⭐

@ngoldbaum ngoldbaum merged commit 05de73b into numpy:main Feb 28, 2023
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