GeezerVison

Walkthrough

Refactors winreg error handling: os_error_from_windows_code now takes only (vm, code); SetValueEx's value_name changed to Option<String>; call sites updated to use unified OS error mapping and removed per-call formatted error messages.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Rewrite of winreg module and bump to 3.13.2 #5594: Modifies the same winreg.rs functions—centralizing os_error_from_windows_code and updating SetValueEx/value-name handling.

Poem

🐰 I hopped through code with nimble paws,
Streamlined errors, trimmed the claws,
One code, one call, no stringy fray,
Registry dances tidy today!

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• 📝 Generate docstrings

crates/vm/src/stdlib/winreg.rs (2)

297-298: Remaining call sites still use the old vm.new_os_error(format!("error code: ...")) pattern.

Since this PR's goal is to fix winreg error handling, consider migrating the remaining ~15 call sites (e.g., ConnectRegistry, CreateKey, CreateKeyEx, DeleteKey, DeleteValue, DeleteKeyEx, FlushKey, LoadKey, QueryInfoKey, SaveKey, DisableReflectionKey, EnableReflectionKey, QueryReflectionKey, EnumValue, PyHkey::Close) to use os_error_from_windows_code(vm, res as i32) for consistent, human-readable Windows error descriptions.

Example fix for ConnectRegistry (same pattern applies elsewhere)

         if res == 0 {
             Ok(PyHkey::new(ret_key))
         } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
+            Err(os_error_from_windows_code(vm, res as i32))
         }

---

278-310: Duplicated logic across Some/None branches.

The two branches differ only in the pointer value passed for computer_name but duplicate the entire FFI call and error handling.

Suggested consolidation

 fn ConnectRegistry(
     computer_name: Option<String>,
     key: PyRef<PyHkey>,
     vm: &VirtualMachine,
 ) -> PyResult<PyHkey> {
-    if let Some(computer_name) = computer_name {
-        let mut ret_key = core::ptr::null_mut();
-        let wide_computer_name = computer_name.to_wide_with_nul();
-        let res = unsafe {
-            Registry::RegConnectRegistryW(
-                wide_computer_name.as_ptr(),
-                key.hkey.load(),
-                &mut ret_key,
-            )
-        };
-        if res == 0 {
-            Ok(PyHkey::new(ret_key))
-        } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
-        }
-    } else {
-        let mut ret_key = core::ptr::null_mut();
-        let res = unsafe {
-            Registry::RegConnectRegistryW(core::ptr::null_mut(), key.hkey.load(), &mut ret_key)
-        };
-        if res == 0 {
-            Ok(PyHkey::new(ret_key))
-        } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
-        }
-    }
+    let wide_name = computer_name.as_deref().map(|s| s.to_wide_with_nul());
+    let name_ptr = wide_name
+        .as_deref()
+        .map_or(core::ptr::null(), |s| s.as_ptr());
+    let mut ret_key = core::ptr::null_mut();
+    let res = unsafe {
+        Registry::RegConnectRegistryW(name_ptr, key.hkey.load(), &mut ret_key)
+    };
+    if res == 0 {
+        Ok(PyHkey::new(ret_key))
+    } else {
+        Err(os_error_from_windows_code(vm, res as i32))
+    }
 }

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."

• X
• Mastodon
• Reddit
• LinkedIn