Minor cleanups, remove several unwraps

Signed-off-by: Luca Della Vedova <[email protected]>

Author: luca-della-vedova

rclrs/src/parameter.rs

@@ -492,7 +492,7 @@ pub(crate) struct ParameterMap {
 impl ParameterMap {
     /// Validates the requested parameter setting and returns an error if the requested value is
     /// not valid.
-    pub(crate) fn validate_parameter_setting(
+    fn validate_parameter_setting(
         &self,
         name: &str,
         value: RmwParameterValue,

rclrs/src/parameter/service.rs

@@ -43,14 +43,14 @@ impl ParameterService {
                     .names
                     .into_iter()
                     .map(|name| {
-                        // TODO(luca) Remove conversion to string and implement Borrow
-                        let storage = map.storage.get(name.to_cstr().to_str().unwrap())?;
+                        let name = name.to_cstr().to_str().ok()?;
+                        let storage = map.storage.get(name)?;
                         let mut descriptor = match storage {
                             ParameterStorage::Declared(storage) => {
                                 let (integer_range, floating_point_range) =
                                     storage.options.ranges.to_descriptor_ranges();
                                 ParameterDescriptor {
-                                    name,
+                                    name: name.into(),
                                     type_: Default::default(),
                                     description: storage.options.description.clone().into(),
                                     additional_constraints: storage
@@ -65,7 +65,7 @@ impl ParameterService {
                                 }
                             }
                             ParameterStorage::Undeclared(_) => ParameterDescriptor {
-                                name,
+                                name: name.into(),
                                 dynamic_typing: true,
                                 ..Default::default()
                             },
@@ -88,10 +88,8 @@ impl ParameterService {
                     .names
                     .into_iter()
                     .map(|name| {
-                        // TODO(luca) Remove conversion to string and implement Borrow
-                        map.storage
-                            .get(name.to_cstr().to_str().unwrap())
-                            .map(|s| s.to_parameter_type())
+                        let name = name.to_cstr().to_str().ok()?;
+                        map.storage.get(name).map(|s| s.to_parameter_type())
                     })
                     .collect::<Option<_>>()
                     .unwrap_or_default();
@@ -107,9 +105,8 @@ impl ParameterService {
                     .names
                     .into_iter()
                     .map(|name| {
-                        // TODO(luca) Remove conversion to string and implement Borrow
-                        let storage = map.storage.get(name.to_cstr().to_str().unwrap())?;
-                        match storage {
+                        let name = name.to_cstr().to_str().ok()?;
+                        match map.storage.get(name)? {
                             ParameterStorage::Declared(storage) => match &storage.value {
                                 DeclaredValue::Mandatory(v) => {
                                     Some(v.read().unwrap().clone().into())
@@ -160,7 +157,7 @@ impl ParameterService {
                                 let mut prefix = prefix.clone();
                                 prefix.extend(".".chars());
                                 if name.len() > prefix.len()
-                                    && name[0..prefix.len()] == prefix[0..]
+                                    && name.starts_with(&prefix)
                                     && check_parameter_name_depth(&name[prefix.len()..])
                                 {
                                     return true;
@@ -176,12 +173,10 @@ impl ParameterService {
                         let name = name.to_string();
                         if let Some(pos) = name.rfind('.') {
                             return Some(name[0..pos].into());
-                            //return Some(name[0..pos].iter().map(|c| *c as char).collect());
                         }
                         None
                     })
                     .collect();
-                // TODO(luca) populate prefixes in result
                 ListParameters_Response {
                     result: ListParametersResult {
                         names,
@@ -199,20 +194,25 @@ impl ParameterService {
                     .parameters
                     .into_iter()
                     .map(|param| {
-                        let name = param.name.to_cstr().to_str().unwrap();
-                        let value = map.validate_parameter_setting(name, param.value)?;
-                        map.store_parameter(name.into(), value);
-                        Ok(())
-                    })
-                    .map(|res| match res {
-                        Ok(()) => SetParametersResult {
-                            successful: true,
-                            reason: Default::default(),
-                        },
-                        Err(e) => SetParametersResult {
-                            successful: false,
-                            reason: e,
-                        },
+                        let Ok(name) = param.name.to_cstr().to_str() else {
+                            return SetParametersResult {
+                                successful: false,
+                                reason: "Failed parsing into UTF-8".into(),
+                            };
+                        };
+                        match map.validate_parameter_setting(name, param.value) {
+                            Ok(value) => {
+                                map.store_parameter(name.into(), value);
+                                SetParametersResult {
+                                    successful: true,
+                                    reason: Default::default(),
+                                }
+                            }
+                            Err(e) => SetParametersResult {
+                                successful: false,
+                                reason: e.into(),
+                            },
+                        }
                     })
                     .collect();
                 SetParameters_Response { results }
@@ -226,7 +226,9 @@ impl ParameterService {
                     .parameters
                     .into_iter()
                     .map(|param| {
-                        let name = param.name.to_cstr().to_str().unwrap();
+                        let Ok(name) = param.name.to_cstr().to_str() else {
+                            return Err("Failed parsing into UTF-8".into());
+                        };
                         let value = map.validate_parameter_setting(name, param.value)?;
                         Ok((name.into(), value))
                     })

rclrs/src/parameter/value.rs

@@ -369,7 +369,6 @@ impl From<ParameterValue> for RmwParameterValue {
             },
             ParameterValue::StringArray(v) => RmwParameterValue {
                 type_: ParameterType::PARAMETER_STRING_ARRAY,
-                // TODO(luca) check if there is a more efficient way to perform this conversion
                 string_array_value: v
                     .iter()
                     .map(|v| v.clone().into())

rosidl_runtime_rs/src/string.rs

@@ -1,9 +1,9 @@
+use std::borrow::Borrow;
 use std::cmp::Ordering;
 use std::ffi::CStr;
 use std::fmt::{self, Debug, Display};
 use std::hash::{Hash, Hasher};
 use std::ops::{Deref, DerefMut};
-use std::sync::Arc;
 
 #[cfg(feature = "serde")]
 mod serde;
@@ -302,31 +302,17 @@ string_impl!(
     rosidl_runtime_c__U16String__Sequence__copy
 );
 
-impl From<&str> for String {
-    fn from(s: &str) -> Self {
-        let mut msg = Self {
-            data: std::ptr::null_mut(),
-            size: 0,
-            capacity: 0,
-        };
-        // SAFETY: It's okay to pass a non-zero-terminated string here since assignn uses the
-        // specified length and will append the 0 byte to the dest string itself.
-        if !unsafe {
-            rosidl_runtime_c__String__assignn(&mut msg as *mut _, s.as_ptr() as *const _, s.len())
-        } {
-            panic!("rosidl_runtime_c__String__assignn failed");
-        }
-        msg
-    }
-}
-
-impl From<Arc<str>> for String {
-    fn from(s: Arc<str>) -> Self {
+impl<T> From<T> for String
+where
+    T: Borrow<str>,
+{
+    fn from(s: T) -> Self {
         let mut msg = Self {
             data: std::ptr::null_mut(),
             size: 0,
             capacity: 0,
         };
+        let s = s.borrow();
         // SAFETY: It's okay to pass a non-zero-terminated string here since assignn uses the
         // specified length and will append the 0 byte to the dest string itself.
         if !unsafe {