Skip to content

feat: implement ngx_queue_t wrapper and an owning Queue on top of it #181

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 2 commits into from
Jul 22, 2025

Conversation

bavshin-f5
Copy link
Member

Also, address feedback for the RbTreeMap implementation from #164

@bavshin-f5 bavshin-f5 requested a review from Copilot July 21, 2025 16:55
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a ngx_queue_t wrapper and an owning Queue type, and addresses feedback for the RbTreeMap implementation. The changes provide comprehensive queue/linked list functionality alongside improvements to the existing red-black tree implementation.

  • Adds a new queue.rs module with NgxQueue wrapper and owning Queue type
  • Refactors RbTreeMap to use a new NgxRbTree wrapper for better abstraction
  • Updates module exports to include the new queue functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/collections/rbtree.rs Refactors RbTreeMap to use NgxRbTree wrapper, adds NgxRbTreeEntry trait, renames iterator types
src/collections/queue.rs New module implementing NgxQueue wrapper and owning Queue with comprehensive iterator support
src/collections/mod.rs Exports new Queue type and queue module
Comments suppressed due to low confidence (10)

src/collections/rbtree.rs:34

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'node' to better reflect that it's an rbtree node pointer.
    fn from_rbtree_node(q: NonNull<ngx_rbtree_node_t>) -> NonNull<Self>;

src/collections/rbtree.rs:40

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'node' to be consistent with the trait definition and improve clarity.
    fn from_rbtree_node(q: NonNull<ngx_rbtree_node_t>) -> NonNull<Self> {

src/collections/rbtree.rs:41

  • [nitpick] Return value should match the renamed parameter. If parameter is renamed to 'node', this should return 'node'.
        q

src/collections/rbtree.rs:204

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'node' for consistency with the trait definition.
    fn from_rbtree_node(q: NonNull<ngx_rbtree_node_t>) -> NonNull<Self> {

src/collections/rbtree.rs:205

  • [nitpick] Usage should match the renamed parameter. If parameter is renamed to 'node', this should use 'node' instead of 'q'.
        unsafe { ngx_rbtree_data!(q, Self, node) }

src/collections/queue.rs:32

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'queue_node' or 'node' to better reflect its purpose.
    fn from_queue(q: NonNull<ngx_queue_t>) -> NonNull<Self>;

src/collections/queue.rs:38

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'queue_node' or 'node' for consistency with the trait definition.
    fn from_queue(q: NonNull<ngx_queue_t>) -> NonNull<Self> {

src/collections/queue.rs:39

  • [nitpick] Return value should match the renamed parameter. If parameter is renamed, this should return the new parameter name.
        q

src/collections/queue.rs:376

  • [nitpick] Parameter name 'q' is ambiguous. Consider renaming it to 'queue_node' or 'node' for consistency with the trait definition.
    fn from_queue(q: NonNull<ngx_queue_t>) -> NonNull<Self> {

src/collections/queue.rs:377

  • [nitpick] Usage should match the renamed parameter. If parameter is renamed, this macro call should use the new parameter name.
        unsafe { ngx_queue_data!(q, Self, queue) }

@bavshin-f5 bavshin-f5 force-pushed the collections branch 3 times, most recently from 03a7923 to 1bc3ac7 Compare July 21, 2025 21:45
Copy link
Contributor

@ensh63 ensh63 left a comment

Choose a reason for hiding this comment

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

LGFM

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 great.

@bavshin-f5 bavshin-f5 merged commit 95424ad into nginx:main Jul 22, 2025
15 checks passed
@bavshin-f5 bavshin-f5 deleted the collections branch July 22, 2025 17:32
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