diff --git a/src/errors.rs b/src/errors.rs index 84d5e39..8d141ff 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,6 +1,7 @@ use crate::c_abi; use crate::FileMode; use crate::Frame; +use std::convert::TryInto; use std::error::Error as StdError; use std::path::{Path, PathBuf}; @@ -27,6 +28,14 @@ pub enum Error { /// A path could not be converted to &CStr because it had a null byte NullInStr(std::ffi::NulError), CouldNotCheckNAtoms(Box), + /// Step was out of range for usize on this platform + StepSizeOutOfRange(i32), + /// A numeric cast from `value` failed during `task` + NumericCastFailed { + source: std::num::TryFromIntError, + task: ErrorTask, + value: usize, + }, } impl Error { @@ -64,6 +73,7 @@ impl std::error::Error for Error { match &self { NullInStr(err) => Some(err), CouldNotCheckNAtoms(err) => Some(err.as_ref()), + NumericCastFailed { source, .. } => Some(source), _ => None, } } @@ -105,7 +115,7 @@ impl From<(&Frame, usize)> for Error { impl std::fmt::Display for Error { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { use Error::*; - match &self { + match self { CApiError { code, task } => write!( f, "Error while {task}: C API returned error code {code}", @@ -122,10 +132,16 @@ impl std::fmt::Display for Error { } InvalidOsStr => write!(f, "Paths must be valid unicode on this platform"), NullInStr(_err) => write!(f, "Paths cannot include null bytes"), - CouldNotCheckNAtoms(_err) => write!( + CouldNotCheckNAtoms(_err) => { + write!(f, "Failed to read number of atoms in trajectory file") + } + StepSizeOutOfRange(n) => write!(f, "Step {} does not fit in usize on this platform", n), + NumericCastFailed { value, task, .. } => write!( f, - "Failed to read number of atoms in trajectory file" - ) + "Numeric cast from {value} failed while {task}", + value = value, + task = task + ), } } } @@ -200,9 +216,13 @@ impl ErrorCode { } } -impl From for ErrorCode { - fn from(code: c_abi::xdrfile::BindgenTy1) -> Self { - match code { +impl From for ErrorCode +where + T: TryInto, + >::Error: std::fmt::Debug, +{ + fn from(code: T) -> Self { + match code.try_into().expect("C API return code was out of range") { c_abi::xdrfile::exdrOK => Self::ExdrOk, c_abi::xdrfile::exdrHEADER => Self::ExdrHeader, c_abi::xdrfile::exdrSTRING => Self::ExdrString, diff --git a/src/frame.rs b/src/frame.rs index 6cabb69..f8b6c50 100644 --- a/src/frame.rs +++ b/src/frame.rs @@ -28,9 +28,7 @@ impl Default for Frame { impl Frame { /// Creates an empty frame with a capacity of 0 pub fn new() -> Frame { - Frame { - ..Default::default() - } + Default::default() } /// Creates a frame with the given capacity @@ -55,6 +53,11 @@ impl Frame { /// Length of the frame (number of atoms) pub fn len(self: &Frame) -> usize { + self.num_atoms() + } + + /// The number of atoms in the frame + pub fn num_atoms(self: &Frame) -> usize { self.coords.len() } diff --git a/src/lib.rs b/src/lib.rs index c2cdac2..fda4610 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,6 +77,7 @@ use c_abi::xdrfile_xtc; use lazy_init::Lazy; use std::cell::Cell; +use std::convert::{TryFrom, TryInto}; use std::ffi::CString; use std::io; use std::path::{Path, PathBuf}; @@ -108,6 +109,14 @@ fn path_to_cstring(path: impl AsRef) -> Result { Ok(CString::new(s)?) } +fn to_i32(value: usize, task: ErrorTask) -> Result { + value.try_into().map_err(|e| Error::NumericCastFailed { + source: e, + value, + task, + }) +} + /// Convert an error code from a C call to an Error /// /// `code` should be an integer return code returned from the C API. @@ -250,7 +259,7 @@ impl Trajectory for XTCTrajectory { .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))?; if num_atoms != frame.coords.len() { Err((&*frame, num_atoms))?; - } + }; unsafe { // C lib requires an i32 to be passed, but step is exposed it as u32 @@ -258,19 +267,18 @@ impl Trajectory for XTCTrajectory { // variable to pass to read_xtc and cast it afterwards to u32 let code = xdrfile_xtc::read_xtc( self.handle.xdrfile, - num_atoms as i32, + to_i32(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut frame.box_vector, frame.coords.as_mut_ptr(), &mut self.precision.get(), - ) as u32; - frame.step = step as usize; + ); if let Some(err) = check_code(code, ErrorTask::Read) { - Err(err) - } else { - Ok(()) + return Err(err); } + frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?; + Ok(()) } } @@ -278,13 +286,13 @@ impl Trajectory for XTCTrajectory { unsafe { let code = xdrfile_xtc::write_xtc( self.handle.xdrfile, - frame.len() as i32, - frame.step as i32, + to_i32(frame.len(), ErrorTask::Write)?, + to_i32(frame.step, ErrorTask::Write)?, frame.time, frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], frame.coords[..].as_ptr() as *mut [f32; 3], 1000.0, - ) as u32; + ); if let Some(err) = check_code(code, ErrorTask::Write) { Err(err) } else { @@ -295,7 +303,7 @@ impl Trajectory for XTCTrajectory { fn flush(&mut self) -> Result<()> { unsafe { - let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32; + let code = xdr_seek::xdr_flush(self.handle.xdrfile); if let Some(err) = check_code(code, ErrorTask::Read) { Err(err) } else { @@ -312,15 +320,15 @@ impl Trajectory for XTCTrajectory { unsafe { let path = path_to_cstring(&self.handle.path)?; let path_p = path.into_raw(); - let code = - xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const i32) as u32; + let code = xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const i32); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(num_atoms as usize) + Ok(usize::try_from(num_atoms) + .expect("Number of atoms in file does not fit in usize")) } } }) @@ -391,7 +399,7 @@ impl Trajectory for TRRTrajectory { // Similar for lambda. let code = xdrfile_trr::read_trr( self.handle.xdrfile, - num_atoms as i32, + to_i32(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut lambda, @@ -399,14 +407,12 @@ impl Trajectory for TRRTrajectory { frame.coords.as_mut_ptr(), std::ptr::null_mut(), std::ptr::null_mut(), - ) as u32; - - frame.step = step as usize; + ); if let Some(err) = check_code(code, ErrorTask::Read) { - Err(err) - } else { - Ok(()) + return Err(err); } + frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?; + Ok(()) } } @@ -414,15 +420,15 @@ impl Trajectory for TRRTrajectory { unsafe { let code = xdrfile_trr::write_trr( self.handle.xdrfile, - frame.len() as i32, - frame.step as i32, + to_i32(frame.len(), ErrorTask::Write)?, + to_i32(frame.step, ErrorTask::Write)?, frame.time, 0.0, frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], frame.coords[..].as_ptr() as *mut [f32; 3], std::ptr::null_mut(), std::ptr::null_mut(), - ) as u32; + ); if let Some(err) = check_code(code, ErrorTask::Write) { Err(err) } else { @@ -433,7 +439,7 @@ impl Trajectory for TRRTrajectory { fn flush(&mut self) -> Result<()> { unsafe { - let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32; + let code = xdr_seek::xdr_flush(self.handle.xdrfile); if let Some(err) = check_code(code, ErrorTask::Flush) { Err(err) } else { @@ -449,15 +455,15 @@ impl Trajectory for TRRTrajectory { unsafe { let path = path_to_cstring(&self.handle.path)?; let path_p = path.into_raw(); - let code = - xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const i32) as u32; + let code = xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const i32); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(num_atoms as usize) + Ok(usize::try_from(num_atoms) + .expect("Number of atoms in file does not fit in usize")) } } }) @@ -530,7 +536,7 @@ mod tests { let tempfile = NamedTempFile::new().expect("Could not create temporary file"); let tmp_path = tempfile.path(); - let natoms: u32 = 2; + let natoms = 2; let frame = Frame { step: 5, time: 2.0, @@ -545,7 +551,7 @@ mod tests { } f.flush()?; - let mut new_frame = Frame::with_len(natoms as usize); + let mut new_frame = Frame::with_len(natoms); let mut f = TRRTrajectory::open_read(tmp_path)?; // let num_atoms = f.get_num_atoms()?; // assert_eq!(num_atoms, natoms); @@ -744,7 +750,7 @@ mod tests { let tempfile = NamedTempFile::new()?; let tmp_path = tempfile.path(); - let natoms: u32 = 2; + let natoms = 2; let frame = Frame { step: 5, time: 2.0, @@ -755,7 +761,7 @@ mod tests { f.write(&frame)?; f.flush()?; - let mut new_frame = Frame::with_len(natoms as usize); + let mut new_frame = Frame::with_len(natoms); let mut f = XTCTrajectory::open_read(tmp_path)?; f.read(&mut new_frame)?; diff --git a/tests/integration.rs b/tests/integration.rs index c7fb158..dcf67ef 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -8,7 +8,7 @@ mod integration { fn test_use_library() -> Result<()> { let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; let num_atoms = trj.get_num_atoms()?; - let mut frame = Frame::with_len(num_atoms as usize); + let mut frame = Frame::with_len(num_atoms); trj.read(&mut frame)?; trj.read(&mut frame)?; @@ -21,7 +21,7 @@ mod integration { let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; let frames: Result>> = trj.into_iter().collect(); for (idx, frame) in frames?.iter().enumerate() { - assert_eq!(frame.step as usize, idx + 1); + assert_eq!(frame.step, idx + 1); } Ok(()) }