Skip to content

ngx::core::Pool's methods are &self, not &mut self #196

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 1 commit into from
Aug 18, 2025

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Aug 18, 2025

The interior pointer to ngx_pool_t provides the mutability we need here - there is no requirement for the use of the ngx_pool_t to be unique, so restricting the methods to &mut self just adds additional difficulty without any safety benefit.

Closes #192

Checklist

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

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

@bavshin-f5
Copy link
Member

Makes sense, thanks!
Just reminding that we use conventional commits in this repo. I'd appreciate if you reword the commit message like refactor: take shared ref to self in ngx::core::Pool methods.

The interior ngx_pool_t provides the mutability we need here

interior pointer to ngx_pool_t?

The interior *mut ngx_pool_t provides the mutability we need here -
there is no requirement for the use of the ngx_pool_t to be unique, so
restricting the methods to &mut self just adds additional difficulty
without any safety benefit.
@pchickey pchickey force-pushed the pch/pool_methods_no_mut branch from ef271ab to 72445f5 Compare August 18, 2025 23:43
@pchickey
Copy link
Contributor Author

@bavshin-f5 Both fixed, thanks for the reminder.

@bavshin-f5 bavshin-f5 merged commit 1f4b572 into nginx:main Aug 18, 2025
15 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.

Pool and Allocator are difficult to use together
2 participants