[PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Onur Özkan]

Refactors parts of the get() and cursor_lower_bound()
traversal logic to minimize the scope of unsafe blocks
and avoid duplicating same safety comments.

One of the removed comments was also misleading:

    // SAFETY: `node` is a non-null node...
    Ordering::Equal => return Some(unsafe { &(*this).value }),

as `node` should have been `this`.

No functional changes intended; this is purely a safety
improvement that reduces the amount of unsafe blocks
while keeping all invariants intact.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/rbtree.rs | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index b8fe6be6fcc4..d44f305e5161 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -384,14 +384,17 @@ pub fn get(&self, key: &K) -> Option<&V> {
             // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
             // point to the links field of `Node<K, V>` objects.
             let this = unsafe { container_of!(node, Node<K, V>, links) };
+
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
-            node = match key.cmp(unsafe { &(*this).key }) {
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Less => unsafe { (*node).rb_left },
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Greater => unsafe { (*node).rb_right },
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Equal => return Some(unsafe { &(*this).value }),
+            let this_ref = unsafe { &*this };
+
+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let node_ref = unsafe { &*node };
+
+            node = match key.cmp(&this_ref.key) {
+                Ordering::Less => node_ref.rb_left,
+                Ordering::Greater => node_ref.rb_right,
+                Ordering::Equal => return Some(&this_ref.value),
             }
         }
         None
@@ -433,17 +436,17 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
             let this = unsafe { container_of!(node, Node<K, V>, links) };
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
             let this_key = unsafe { &(*this).key };
+
             // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-            let left_child = unsafe { (*node).rb_left };
-            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-            let right_child = unsafe { (*node).rb_right };
+            let node_ref = unsafe { &*node };
+
             match key.cmp(this_key) {
                 Ordering::Equal => {
                     best_match = NonNull::new(this);
                     break;
                 }
                 Ordering::Greater => {
-                    node = right_child;
+                    node = node_ref.rb_right;
                 }
                 Ordering::Less => {
                     let is_better_match = match best_match {
@@ -457,7 +460,7 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
                     if is_better_match {
                         best_match = NonNull::new(this);
                     }
-                    node = left_child;
+                    node = node_ref.rb_left;
                 }
             };
         }
-- 
2.51.2

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Charalampos Mitrodimas]

Onur Özkan <work@onurozkan.dev> writes:

> Refactors parts of the get() and cursor_lower_bound()
> traversal logic to minimize the scope of unsafe blocks
> and avoid duplicating same safety comments.
>
> One of the removed comments was also misleading:
>
>     // SAFETY: `node` is a non-null node...
>     Ordering::Equal => return Some(unsafe { &(*this).value }),
>
> as `node` should have been `this`.
>
> No functional changes intended; this is purely a safety
> improvement that reduces the amount of unsafe blocks
> while keeping all invariants intact.
>
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  rust/kernel/rbtree.rs | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>

This is a nice change to have if it reaches the general consensus.

Acked-By: Charalampos Mitrodimas <charmitro@posteo.net>

--
C. Mitrodimas