Rewrite WUtils bagtree.c in Rust #4

trurl · 2025-10-28T21:27:02-04:00

trurl commented

2025-10-28 21:27:02 -04:00

In this case, we can replace the red-black tree with a sorted Vec. That makes rewriting it pretty simple, although it is yet another thing that we actually want to get rid of at some point.

trurl added 2 commits 2025-10-28 21:27:02 -04:00

Prune the WMBag API in preparation for rewriting it in Rust. 0f748de74f

Rewrite WMBag in Rust. d4443114b8

We should eventually get rid of this entirely, in favor of something along the
lines of a sorted Vec that is fully typed (so the payload isn't just a void*).

trurl requested review from cross 2025-10-28 21:27:15 -04:00

cross reviewed 2025-10-29 15:44:16 -04:00

wutil-rs/src/bag.rs Outdated

						
				@@ -0,0 +15,4 @@

				};

				#[derive(Default)]

				pub struct Bag {

cross commented

2025-10-29 15:44:15 -04:00

It feels like you almost don't need this at all; you could do everything you need to do with the Bag abstraction by just making it a newtype over a BTreeMap, and then dereferencing the newtype pointer at the FFI boundary. Something like,

use std::collections::BTreeMap;
use std::ffi::{c_int, c_void};
use std::ptr::{self, NonNull};

pub struct Bag(BTreeMap<c_int, NonNull<c_void>>);

/// Basic constructor. Free with [`WMFreeBag`].
#[unsafe(no_mangle)]
pub unsafe extern "C" fn WMCreateTreeBag() -> *mut Bag {
    let bag = Box::new(Bag(BTreeMap::new()));
    Box::leak(bag)
}

/// Retrieves the value associated with `key`, or null.
#[unsafe(no_mangle)]
pub unsafe extern "C" fn WMGetFromBag(bag: *mut Bag, key: c_int) -> *mut c_void {
    unsafe { bag.as_mut() }
        .and_then(|bag| bag.0.get(&key))
        .map(|p| p.as_ptr())
        .unwrap_or(ptr::null_mut())
}

(And so on.)

It feels like you almost don't need this at all; you could do everything you need to do with the `Bag` abstraction by just making it a newtype over a `BTreeMap`, and then dereferencing the newtype pointer at the FFI boundary. Something like, ``` use std::collections::BTreeMap; use std::ffi::{c_int, c_void}; use std::ptr::{self, NonNull}; pub struct Bag(BTreeMap<c_int, NonNull<c_void>>); /// Basic constructor. Free with [`WMFreeBag`]. #[unsafe(no_mangle)] pub unsafe extern "C" fn WMCreateTreeBag() -> *mut Bag { let bag = Box::new(Bag(BTreeMap::new())); Box::leak(bag) } /// Retrieves the value associated with `key`, or null. #[unsafe(no_mangle)] pub unsafe extern "C" fn WMGetFromBag(bag: *mut Bag, key: c_int) -> *mut c_void { unsafe { bag.as_mut() } .and_then(|bag| bag.0.get(&key)) .map(|p| p.as_ptr()) .unwrap_or(ptr::null_mut()) } ``` (And so on.)

trurl commented

2025-11-01 10:37:46 -04:00

Only putting functionality into the FFI functions makes sense. (I've done similarly in other refactors that you're looking at.) I was thinking to have most operations addressable via completely safe Rust, to reduce the amount of unsafe in unit tests, but for this module the additional lines of code may not be warranted.

I elected to use a sorted Vec instead of a BTreeMap because of the iterator interface: there isn't a proper double-ended BTreeMap iterator in Rust, and WUtils iterators are assumed to be allocation-free. I'm not super inclined to refactor the interface very much right now, so it seems simpler to use an implementation that allows use of an int as an iterator. Unless I'm overlooking a strong reason to use a BTreeMap internally, the sorted Vec seems fine.

Only putting functionality into the FFI functions makes sense. (I've done similarly in other refactors that you're looking at.) I was thinking to have most operations addressable via completely safe Rust, to reduce the amount of unsafe in unit tests, but for this module the additional lines of code may not be warranted. I elected to use a sorted `Vec` instead of a `BTreeMap` because of the iterator interface: there isn't a proper double-ended `BTreeMap` iterator in Rust, and WUtils iterators are assumed to be allocation-free. I'm not super inclined to refactor the interface very much right now, so it seems simpler to use an implementation that allows use of an int as an iterator. Unless I'm overlooking a strong reason to use a `BTreeMap` internally, the sorted `Vec` seems fine.

trurl added 2 commits 2025-11-12 17:05:16 -05:00

Nix dead code that was left commented out. c5951007f5

Move functions on Bag which were only called by FFI code inline. a2940e44ad

cross commented

2025-12-02 11:09:43 -05:00

ok, I think that's fine.

cross approved these changes 2025-12-02 11:09:54 -05:00

trurl force-pushed refactor/wutil-rs-bagtree from a2940e44ad to 0097d1819e

2025-12-08 13:00:18 -05:00

Compare

trurl merged commit 0097d1819e into refactor/wutil-rs

2025-12-08 13:00:34 -05:00

trurl deleted branch refactor/wutil-rs-bagtree

2025-12-08 13:00:34 -05:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vitrine/wmaker#4

Allow edits from maintainers