Skip to content

feat: unsafe updates for raw pointer arguments #38

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

Closed
wants to merge 1 commit into from

Conversation

f5yacobucci
Copy link
Contributor

Dereferencing raw pointers is inherently unsafe. To satisfy clippy these functions and their callers require an unsafe indicator.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • [X ] I have written my commit messages in the Conventional Commits format.
  • [X ] I have read the CONTRIBUTING doc
  • [X ] I have added tests (when possible) that prove my fix is effective or that my feature works
  • [X ] I have checked that all unit tests pass after adding my changes
  • [X ] I have updated necessary documentation
  • [X ] I have rebased my branch onto master
  • [X ] I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Dereferencing raw pointers is inherently unsafe. To satisfy clippy these
functions and their callers require an unsafe indicator.
@f5yacobucci f5yacobucci requested a review from ivanitskiy July 7, 2023 20:22
@f5yacobucci
Copy link
Contributor Author

This changes the safety designation of some functions to satisfy clippy.

Since this is a breaking change for consumers I'd like to discuss alternative approaches, but I couldn't find one in a timely manner.

Copy link
Contributor

@ivanitskiy ivanitskiy left a comment

Choose a reason for hiding this comment

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

Let's merge it! approved, LGTM

unsafe {
copy_nonoverlapping(data.as_ptr(), ptr, data.len());
}
pub unsafe fn str_to_uchar(pool: *mut ngx_pool_t, data: &str) -> *mut u_char {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since nginx-sys changed without backward compatibility, let's bump its version in nginx-sys/Cargo.toml

Copy link
Contributor

Choose a reason for hiding this comment

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

this would require re-publishing it.

@ivanitskiy ivanitskiy mentioned this pull request Aug 28, 2023
7 tasks
@ivanitskiy ivanitskiy closed this Aug 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.

2 participants