Skip to content

Conversation

bavshin-f5
Copy link
Member

ngx_str_t likely does not contain UTF-8, so converting it from/to UTF-8 strings is expensive and useless. from_bytes() and as_bytes() should be a more appropriate interface.

At some point in the future I intend to drop the String/&str conversion methods, as these aren't safe.

Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Looks good.
Added a couple of small questions.

/// # Safety
///
/// The caller must provide a valid pointer to a memory pool.
pub unsafe fn from_bytes(pool: *mut ngx_pool_t, src: &[u8]) -> Option<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

from_string() and from_str(), both return Self, not Option<Self>, shouldn't we change them as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it's worth fixing the str conversion functions. I planned to remove these or at least mark as deprecated.

Use unaligned allocations for strings.
A zero-length `ngx_str_t` is still valid, but the data pointer is not
guaranteed to be valid or initialized in that case.
There's no guarantee we'll be working with a valid UTF-8 data.
`size_t` is transformed to `usize` by default since bindgen 0.61.
As we require much newer version of bindgen, the explicit casts are no
longer necessary.
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Looks good.

@bavshin-f5 bavshin-f5 merged commit 8b6aa4b into nginx:master Oct 23, 2024
7 checks passed
@bavshin-f5 bavshin-f5 deleted the byte-strings branch October 23, 2024 01:53
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