From e6948e0c8608862545c69280c0bd129af6b3a617 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 25 Apr 2025 18:27:35 +0100 Subject: [PATCH] Tweak handling of optionals in the C++ wrapper --- cxx/src/api/plugin.cpp | 12 +++++--- cxx/src/database.rs | 4 +-- cxx/src/game.rs | 4 +-- cxx/src/lib.rs | 68 ++++++++++++++++++++++++++++-------------- cxx/src/metadata.rs | 18 +++-------- cxx/src/plugin.rs | 14 ++------- 6 files changed, 65 insertions(+), 55 deletions(-) diff --git a/cxx/src/api/plugin.cpp b/cxx/src/api/plugin.cpp index 3446e8d1..08c383dc 100644 --- a/cxx/src/api/plugin.cpp +++ b/cxx/src/api/plugin.cpp @@ -43,11 +43,15 @@ std::vector Plugin::GetBashTags() const { } std::optional Plugin::GetCRC() const { - const auto value = plugin_->crc(); - if (value < 0 || value > UINT32_MAX) { + try { + auto optional = plugin_->crc(); + if (optional->is_some()) { + return optional->as_ref(); + } + return std::nullopt; - } else { - return uint32_t(value); + } catch (const ::rust::Error& e) { + std::rethrow_exception(mapError(e)); } } diff --git a/cxx/src/database.rs b/cxx/src/database.rs index 3da75bda..945dbec9 100644 --- a/cxx/src/database.rs +++ b/cxx/src/database.rs @@ -8,9 +8,9 @@ use libloot::{WriteMode, error::DatabaseLockPoisonError}; use libloot_ffi_errors::UnsupportedEnumValueError; use crate::{ - VerboseError, + OptionalPluginMetadata, VerboseError, ffi::EdgeType, - metadata::{Group, Message, OptionalPluginMetadata, PluginMetadata, to_vec_of_unwrapped}, + metadata::{Group, Message, PluginMetadata, to_vec_of_unwrapped}, }; #[derive(Debug)] diff --git a/cxx/src/game.rs b/cxx/src/game.rs index 14261275..85c29594 100644 --- a/cxx/src/game.rs +++ b/cxx/src/game.rs @@ -3,7 +3,7 @@ use std::path::Path; use delegate::delegate; use libloot_ffi_errors::UnsupportedEnumValueError; -use crate::{Plugin, VerboseError, database::Database, ffi::GameType, plugin::OptionalPlugin}; +use crate::{OptionalPlugin, Plugin, VerboseError, database::Database, ffi::GameType}; impl TryFrom for GameType { type Error = UnsupportedEnumValueError; @@ -149,7 +149,7 @@ impl Game { } pub fn plugin(&self, plugin_name: &str) -> Box { - Box::new(self.0.plugin(plugin_name).into()) + Box::new(self.0.plugin(plugin_name).map(Into::into).into()) } pub fn loaded_plugins(&self) -> Vec { diff --git a/cxx/src/lib.rs b/cxx/src/lib.rs index 3b12fd46..61f2161b 100644 --- a/cxx/src/lib.rs +++ b/cxx/src/lib.rs @@ -105,16 +105,16 @@ mod plugin; use database::{Database, Vertex, new_vertex}; use error::{EmptyOptionalError, VerboseError}; +use ffi::OptionalMessageContentRef; use game::{Game, new_game, new_game_with_local_path}; use libloot_ffi_errors::UnsupportedEnumValueError; use metadata::{ - File, Filename, Group, Location, Message, MessageContent, OptionalMessageContentRef, - OptionalPluginMetadata, PluginCleaningData, PluginMetadata, Tag, group_default_name, - message_content_default_language, multilingual_message, new_file, new_filename, new_group, - new_location, new_message, new_message_content, new_plugin_cleaning_data, new_plugin_metadata, - new_tag, select_message_content, + File, Filename, Group, Location, Message, MessageContent, PluginCleaningData, PluginMetadata, + Tag, group_default_name, message_content_default_language, multilingual_message, new_file, + new_filename, new_group, new_location, new_message, new_message_content, + new_plugin_cleaning_data, new_plugin_metadata, new_tag, select_message_content, }; -use plugin::{OptionalPlugin, Plugin}; +use plugin::Plugin; use std::{ ffi::{CString, c_char, c_uchar, c_uint, c_void}, sync::{Mutex, atomic::AtomicPtr}, @@ -124,32 +124,31 @@ use unicase::UniCase; use libloot::set_logging_callback; pub use libloot::{is_compatible, libloot_revision, libloot_version}; -#[derive(Debug)] -pub struct OptionalRef(*const T); - -impl OptionalRef { +impl OptionalMessageContentRef { pub fn is_some(&self) -> bool { - !self.0.is_null() + !self.pointer.is_null() } /// # Safety /// /// This is safe as long as the pointer in the `OptionalRef` is still valid. - pub unsafe fn as_ref(&self) -> Result<&T, EmptyOptionalError> { - if self.0.is_null() { + pub unsafe fn as_ref(&self) -> Result<&MessageContent, EmptyOptionalError> { + if self.pointer.is_null() { Err(EmptyOptionalError) } else { // SAFETY: This is safe as long as self.0 is still valid. - unsafe { Ok(&*self.0) } + unsafe { Ok(&*self.pointer) } } } } -impl From> for OptionalRef { - fn from(value: Option<&T>) -> Self { +impl From> for OptionalMessageContentRef { + fn from(value: Option<&MessageContent>) -> Self { match value { - Some(p) => OptionalRef(p), - None => OptionalRef(std::ptr::null()), + Some(p) => OptionalMessageContentRef { pointer: p }, + None => OptionalMessageContentRef { + pointer: std::ptr::null(), + }, } } } @@ -170,6 +169,18 @@ impl Optional { } } +impl From> for Optional { + fn from(value: Option) -> Self { + Self(value) + } +} + +pub type OptionalPlugin = Optional; + +pub type OptionalPluginMetadata = Optional; + +pub type OptionalCrc = Optional; + fn compare_filenames(lhs: &str, rhs: &str) -> i8 { match UniCase::new(lhs).cmp(&UniCase::new(rhs)) { std::cmp::Ordering::Less => -1, @@ -259,6 +270,19 @@ mod ffi { Fatal, } + #[derive(Debug)] + struct OptionalMessageContentRef { + pointer: *const MessageContent, + } + + extern "Rust" { + pub fn is_some(self: &OptionalMessageContentRef) -> bool; + + // Again, these lifetimes are wrong. + pub unsafe fn as_ref<'a>(self: &'a OptionalMessageContentRef) + -> Result<&'a MessageContent>; + } + extern "Rust" { fn set_log_level(level: LogLevel) -> Result<()>; @@ -269,7 +293,7 @@ mod ffi { fn select_message_content( contents: &[MessageContent], language: &str, - ) -> Box; + ) -> OptionalMessageContentRef; fn compare_filenames(lhs: &str, rhs: &str) -> i8; } @@ -450,7 +474,7 @@ mod ffi { pub fn bash_tags(&self) -> &[String]; // The None case is signalled by -1, all other values fit in u32. - pub fn crc(&self) -> i64; + pub fn crc(&self) -> Box; pub fn is_master(&self) -> bool; @@ -487,12 +511,12 @@ mod ffi { } extern "Rust" { - type OptionalMessageContentRef; + type OptionalCrc; pub fn is_some(&self) -> bool; // Again, these lifetimes are wrong. - pub unsafe fn as_ref<'a>(&'a self) -> Result<&'a MessageContent>; + pub unsafe fn as_ref<'a>(&'a self) -> Result<&'a u32>; } extern "Rust" { diff --git a/cxx/src/metadata.rs b/cxx/src/metadata.rs index f50d709b..7cbf6085 100644 --- a/cxx/src/metadata.rs +++ b/cxx/src/metadata.rs @@ -1,8 +1,8 @@ use delegate::delegate; use crate::{ - Optional, OptionalRef, UnsupportedEnumValueError, VerboseError, - ffi::{MessageType, TagSuggestion}, + UnsupportedEnumValueError, VerboseError, + ffi::{MessageType, OptionalMessageContentRef, TagSuggestion}, }; /// # Safety @@ -113,16 +113,14 @@ impl From> for libloot::metadata::MessageContent { } } -pub type OptionalMessageContentRef = OptionalRef; - pub fn select_message_content( contents: &[MessageContent], language: &str, -) -> Box { +) -> OptionalMessageContentRef { let option = libloot::metadata::select_message_content(MessageContent::unwrap_slice(contents), language); - Box::new(option.map(MessageContent::wrap_ref).into()) + option.map(MessageContent::wrap_ref).into() } #[derive(Clone, Debug)] @@ -379,14 +377,6 @@ impl From> for libloot::metadata::PluginMetadata { } } -pub type OptionalPluginMetadata = Optional; - -impl From> for Optional { - fn from(value: Option) -> Self { - Self(value) - } -} - #[derive(Clone, Debug)] #[repr(transparent)] pub struct File(libloot::metadata::File); diff --git a/cxx/src/plugin.rs b/cxx/src/plugin.rs index a0ddd296..e1a351a4 100644 --- a/cxx/src/plugin.rs +++ b/cxx/src/plugin.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use delegate::delegate; -use crate::{Optional, VerboseError}; +use crate::{OptionalCrc, VerboseError}; #[derive(Debug)] #[repr(transparent)] @@ -25,8 +25,8 @@ impl Plugin { self.0.masters().map_err(Into::into) } - pub fn crc(&self) -> i64 { - self.0.crc().map_or(-1, Into::into) + pub fn crc(&self) -> Box { + Box::new(self.0.crc().into()) } pub fn is_valid_as_light_plugin(&self) -> Result { @@ -77,11 +77,3 @@ impl From> for Plugin { Plugin(value) } } - -pub type OptionalPlugin = Optional; - -impl From>> for Optional { - fn from(value: Option>) -> Self { - Self(value.map(Into::into)) - } -}