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

Add nvim_oxi::string! macro that produces nvim_oxi::Strings #186

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

SCJangra
Copy link
Contributor

Closes #184

crates/types/src/string.rs Outdated Show resolved Hide resolved
crates/types/src/string.rs Outdated Show resolved Hide resolved
crates/types/src/string.rs Outdated Show resolved Hide resolved
crates/types/src/string.rs Show resolved Hide resolved
crates/types/src/string.rs Outdated Show resolved Hide resolved
Comment on lines 172 to 186
// Same as `let n = (required_size as f64 / self.size as f64).ceil()` but using integer
// arithmetic.
//
// `n` is the smallest number that, when multiplied by `self.size`, gives a
// number that is at least equal to `required_size`.
let n = (required_size + self.size - 1) / self.size;
let new_size = self.size * n;

self.data = unsafe {
libc::realloc(self.data as *mut _, new_size)
as *mut ffi::c_char
};

self.size = new_size;
debug_assert!(self.len < self.size)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this strategy is a bit too conservative, and might lead to unnecessary re-allocations. Vec's strategy is to just double the capacity until it's greater than the new length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I thought vec multiplies by current len a certain number of times, but doubling each time makes much more sense, because the initial length might be pretty small.

Copy link
Owner

@noib3 noib3 Oct 24, 2024

Choose a reason for hiding this comment

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

What I meant is that we always keep the capacity a multiple of 2. It starts not as len + 1, but as the smallest power of 2 that's greater or equal than the slice's length (with a minumum of 4). To get the new capacity, we just:

let n = (required_cap - 1).ilog2() + 1;
let new_cap = 2_usize.pow(n);

or even:

loop {
    self.cap *= 2;
    if required_cap <= self.cap {
        break;
    }
}

@SCJangra
Copy link
Contributor Author

SCJangra commented Oct 1, 2024

@noib3 I've pushed the changes, let me if anything else needs to be updated.

@noib3
Copy link
Owner

noib3 commented Oct 24, 2024

Hi, sorry for the long delay.

If you still want to finish the PR, I'd merge it after this is addressed and all the commits are squashed.

types: rename `size` field to `len` in `String`

types: remove `#[repr(C)]` from `StringBuilder`

types: use the `String` type itself instead of redefining the fields in
`StringBuilder`

types: a less conservative reallocation for `StringBuilder`

types: update comment on deallocation in `StringBuilder`

types(string): keep the size of allocation in string to a power of 2.
@SCJangra
Copy link
Contributor Author

Hi, sorry for the long delay.

No problem

I'd merge it after #186 (comment) is addressed and all the commits are squashed.

Done

@noib3 noib3 merged commit 2fc7313 into noib3:main Nov 1, 2024
12 checks passed
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.

A format! like macro to create nvim_oxi::Strings
2 participants