From 58719dad5d6425d889ecf73e9e46aa2063bfbcb6 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 11 Nov 2020 18:42:21 +1100 Subject: [PATCH 01/17] Removed all fallible casts (except in tests) --- src/errors.rs | 34 ++++++++++++++++----- src/frame.rs | 9 ++++-- src/lib.rs | 70 ++++++++++++++++++++++++-------------------- tests/integration.rs | 4 +-- 4 files changed, 73 insertions(+), 44 deletions(-) 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(()) } From 1192188cb0ad8e684edddfda37c272be5d215d42 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 11 Nov 2020 18:58:32 +1100 Subject: [PATCH 02/17] Test for to_i32 --- src/lib.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index fda4610..5d7b2c3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -120,7 +120,7 @@ fn to_i32(value: usize, task: ErrorTask) -> Result { /// Convert an error code from a C call to an Error /// /// `code` should be an integer return code returned from the C API. -/// If `code` indicates the function returned successfully, Nothing is returned; +/// If `code` indicates the function returned successfully, None is returned; /// otherwise, the code is converted into the appropriate `Error`. pub fn check_code(code: impl Into, task: ErrorTask) -> Option { let code: ErrorCode = code.into(); @@ -798,4 +798,22 @@ mod tests { assert!(check_code(code, ErrorTask::Read).is_some()); } } + + #[test] + fn test_to_i32() -> Result<()> { + assert_eq!(24234_i32, to_i32(24234_usize, ErrorTask::Read)?); + + let try_from_int_err = match u8::try_from(-1) { + Err(e) => e, + _ => panic!("Conversion from -1 to u8 succeeded"), + }; + let expected = Error::NumericCastFailed { + source: try_from_int_err, + task: ErrorTask::Write, + value: 3_294_967_295_usize, + }; + assert_eq!(Err(expected), to_i32(3_294_967_295_usize, ErrorTask::Write)); + + Ok(()) + } } From ecccd6aca4df8813b8082c2cb6f725ae7bec34bb Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sat, 14 Nov 2020 13:06:53 +1100 Subject: [PATCH 03/17] Fix a few casts after rebase --- src/lib.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 5d7b2c3..62fe0d0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -80,9 +80,8 @@ use std::cell::Cell; use std::convert::{TryFrom, TryInto}; use std::ffi::CString; use std::io; -use std::path::{Path, PathBuf}; -use std::convert::TryInto; use std::io::SeekFrom; +use std::path::{Path, PathBuf}; #[derive(Debug, Clone, PartialEq)] pub enum FileMode { @@ -179,12 +178,15 @@ impl XDRFile { impl io::Seek for XDRFile { fn seek(&mut self, pos: io::SeekFrom) -> io::Result { let (whence, pos) = match pos { - SeekFrom::Start(u) => (0, u as i64), + SeekFrom::Start(u) => ( + 0, + i64::try_from(u).expect("Seek position did not fit in i64"), + ), SeekFrom::Current(i) => (1, i), SeekFrom::End(i) => (2, i), }; unsafe { - let code = xdr_seek::xdr_seek(self.xdrfile, pos, whence) as u32; + let code = xdr_seek::xdr_seek(self.xdrfile, pos, whence); match check_code(code, ErrorTask::Seek) { None => Ok(self.tell()), Some(err) => Err(io::Error::new(io::ErrorKind::Other, err)), @@ -488,9 +490,9 @@ impl io::Seek for TRRTrajectory { mod tests { use super::*; - use tempfile::NamedTempFile; use std::io::Seek; use std::io::Write; + use tempfile::NamedTempFile; #[test] fn test_read_write_xtc() -> Result<()> { From 8f34cdfcdf683c1c12d2b6e730ab896ad26c3db9 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 16:48:52 +1100 Subject: [PATCH 04/17] no more BindgenTy1 --- src/c_abi/xdrfile.rs | 29 ++++++++++++++--------------- src/c_abi/xdrfile_trr.rs | 6 +++--- src/c_abi/xdrfile_xtc.rs | 6 +++--- src/errors.rs | 13 ++++--------- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/src/c_abi/xdrfile.rs b/src/c_abi/xdrfile.rs index 8c8992c..7c4ad01 100644 --- a/src/c_abi/xdrfile.rs +++ b/src/c_abi/xdrfile.rs @@ -6,21 +6,20 @@ pub struct XDRFILE { _unused: [u8; 0], } -pub type BindgenTy1 = u32; -pub const exdrOK: BindgenTy1 = 0; -pub const exdrHEADER: BindgenTy1 = 1; -pub const exdrSTRING: BindgenTy1 = 2; -pub const exdrDOUBLE: BindgenTy1 = 3; -pub const exdrINT: BindgenTy1 = 4; -pub const exdrFLOAT: BindgenTy1 = 5; -pub const exdrUINT: BindgenTy1 = 6; -pub const exdr3DX: BindgenTy1 = 7; -pub const exdrCLOSE: BindgenTy1 = 8; -pub const exdrMAGIC: BindgenTy1 = 9; -pub const exdrNOMEM: BindgenTy1 = 10; -pub const exdrENDOFFILE: BindgenTy1 = 11; -pub const exdrFILENOTFOUND: BindgenTy1 = 12; -pub const exdrNR: BindgenTy1 = 13; +pub const exdrOK: i32 = 0; +pub const exdrHEADER: i32 = 1; +pub const exdrSTRING: i32 = 2; +pub const exdrDOUBLE: i32 = 3; +pub const exdrINT: i32 = 4; +pub const exdrFLOAT: i32 = 5; +pub const exdrUINT: i32 = 6; +pub const exdr3DX: i32 = 7; +pub const exdrCLOSE: i32 = 8; +pub const exdrMAGIC: i32 = 9; +pub const exdrNOMEM: i32 = 10; +pub const exdrENDOFFILE: i32 = 11; +pub const exdrFILENOTFOUND: i32 = 12; +pub const exdrNR: i32 = 13; extern "C" { pub static mut exdr_message: [*mut ::std::os::raw::c_char; 13usize]; diff --git a/src/c_abi/xdrfile_trr.rs b/src/c_abi/xdrfile_trr.rs index a7eae5b..d5c87b7 100644 --- a/src/c_abi/xdrfile_trr.rs +++ b/src/c_abi/xdrfile_trr.rs @@ -65,7 +65,7 @@ mod tests { unsafe { let code = read_trr_nframes(path.as_ptr(), &mut nframes); - assert!(code as u32 == exdrOK); + assert!(code == exdrOK); } assert!(nframes == 38, "{:?}", nframes); Ok(()) @@ -106,7 +106,7 @@ mod tests { v.as_ptr() as *mut Rvec, f.as_ptr() as *mut Rvec, ); - assert!(write_code as u32 == exdrOK); + assert!(write_code == exdrOK); xdrfile_close(xdr); } @@ -134,7 +134,7 @@ mod tests { v2.as_ptr() as *mut Rvec, f2.as_ptr() as *mut Rvec, ); - assert!(read_code as u32 == exdrOK); + assert!(read_code == exdrOK); xdrfile_close(xdr); } diff --git a/src/c_abi/xdrfile_xtc.rs b/src/c_abi/xdrfile_xtc.rs index ab8ba8b..a002604 100644 --- a/src/c_abi/xdrfile_xtc.rs +++ b/src/c_abi/xdrfile_xtc.rs @@ -61,7 +61,7 @@ mod tests { unsafe { let code = read_xtc_nframes(path.as_ptr(), &mut nframes); - assert!(code as u32 == exdrOK); + assert!(code == exdrOK); } assert!(nframes == 38, "{:?}", nframes); Ok(()) @@ -96,7 +96,7 @@ mod tests { x.as_ptr() as *mut Rvec, 1000.0, ); - assert!(write_code as u32 == exdrOK); + assert!(write_code == exdrOK); xdrfile_close(xdr); } @@ -119,7 +119,7 @@ mod tests { x2.as_ptr() as *mut Rvec, &mut prec, ); - assert!(read_code as u32 == exdrOK); + assert!(read_code == exdrOK); xdrfile_close(xdr); } diff --git a/src/errors.rs b/src/errors.rs index 8d141ff..797f0b3 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,7 +1,6 @@ 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}; @@ -206,7 +205,7 @@ pub enum ErrorCode { /// Failed to seek within file ExdrNr, /// Something unexpected happened - UnmatchedCode(c_abi::xdrfile::BindgenTy1), + UnmatchedCode(i32), } impl ErrorCode { @@ -216,13 +215,9 @@ impl ErrorCode { } } -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") { +impl From for ErrorCode { + fn from(code: i32) -> Self { + match code { c_abi::xdrfile::exdrOK => Self::ExdrOk, c_abi::xdrfile::exdrHEADER => Self::ExdrHeader, c_abi::xdrfile::exdrSTRING => Self::ExdrString, From 8f310f3fe78f3267e13097da330dc75a9261c5aa Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 16:54:42 +1100 Subject: [PATCH 05/17] Added from type info to cast errors --- src/errors.rs | 8 ++++---- src/lib.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 797f0b3..5c439c2 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -30,7 +30,7 @@ pub enum Error { /// Step was out of range for usize on this platform StepSizeOutOfRange(i32), /// A numeric cast from `value` failed during `task` - NumericCastFailed { + CastFromI32Failed { source: std::num::TryFromIntError, task: ErrorTask, value: usize, @@ -72,7 +72,7 @@ impl std::error::Error for Error { match &self { NullInStr(err) => Some(err), CouldNotCheckNAtoms(err) => Some(err.as_ref()), - NumericCastFailed { source, .. } => Some(source), + CastFromI32Failed { source, .. } => Some(source), _ => None, } } @@ -135,9 +135,9 @@ impl std::fmt::Display for Error { 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!( + CastFromI32Failed { value, task, .. } => write!( f, - "Numeric cast from {value} failed while {task}", + "Numeric cast from {value}:usize to i32 failed while {task}", value = value, task = task ), diff --git a/src/lib.rs b/src/lib.rs index 62fe0d0..4b329a1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -109,7 +109,7 @@ fn path_to_cstring(path: impl AsRef) -> Result { } fn to_i32(value: usize, task: ErrorTask) -> Result { - value.try_into().map_err(|e| Error::NumericCastFailed { + value.try_into().map_err(|e| Error::CastFromI32Failed { source: e, value, task, @@ -809,7 +809,7 @@ mod tests { Err(e) => e, _ => panic!("Conversion from -1 to u8 succeeded"), }; - let expected = Error::NumericCastFailed { + let expected = Error::CastFromI32Failed { source: try_from_int_err, task: ErrorTask::Write, value: 3_294_967_295_usize, From 7acde7f7dec3b87ccbc011bc2aa1047c09bff8ef Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 17:02:55 +1100 Subject: [PATCH 06/17] Use c_int and c_float behind closed doors rather than i32/f32 --- src/errors.rs | 6 +++--- src/lib.rs | 51 ++++++++++++++++++++++++--------------------------- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 5c439c2..c9d3982 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -30,7 +30,7 @@ pub enum Error { /// Step was out of range for usize on this platform StepSizeOutOfRange(i32), /// A numeric cast from `value` failed during `task` - CastFromI32Failed { + CastToCintFailed { source: std::num::TryFromIntError, task: ErrorTask, value: usize, @@ -72,7 +72,7 @@ impl std::error::Error for Error { match &self { NullInStr(err) => Some(err), CouldNotCheckNAtoms(err) => Some(err.as_ref()), - CastFromI32Failed { source, .. } => Some(source), + CastToCintFailed { source, .. } => Some(source), _ => None, } } @@ -135,7 +135,7 @@ impl std::fmt::Display for Error { 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), - CastFromI32Failed { value, task, .. } => write!( + CastToCintFailed { value, task, .. } => write!( f, "Numeric cast from {value}:usize to i32 failed while {task}", value = value, diff --git a/src/lib.rs b/src/lib.rs index 4b329a1..d08332d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -81,6 +81,7 @@ use std::convert::{TryFrom, TryInto}; use std::ffi::CString; use std::io; use std::io::SeekFrom; +use std::os::raw::{c_float, c_int}; use std::path::{Path, PathBuf}; #[derive(Debug, Clone, PartialEq)] @@ -108,8 +109,8 @@ 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::CastFromI32Failed { +fn to_c_int(value: usize, task: ErrorTask) -> Result { + value.try_into().map_err(|e| Error::CastToCintFailed { source: e, value, task, @@ -222,7 +223,7 @@ pub trait Trajectory { /// Read/Write XTC Trajectories pub struct XTCTrajectory { handle: XDRFile, - precision: Cell, // internal mutability required for read method + precision: Cell, // internal mutability required for read method num_atoms: Lazy>, } @@ -254,7 +255,7 @@ impl XTCTrajectory { impl Trajectory for XTCTrajectory { fn read(&mut self, frame: &mut Frame) -> Result<()> { - let mut step: i32 = 0; + let mut step: c_int = 0; let num_atoms = self .get_num_atoms() @@ -264,12 +265,9 @@ impl Trajectory for XTCTrajectory { }; unsafe { - // C lib requires an i32 to be passed, but step is exposed it as u32 - // (A step cannot be negative, can it?). So we need to create a step - // variable to pass to read_xtc and cast it afterwards to u32 let code = xdrfile_xtc::read_xtc( self.handle.xdrfile, - to_i32(num_atoms, ErrorTask::Read)?, + to_c_int(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut frame.box_vector, @@ -288,8 +286,8 @@ impl Trajectory for XTCTrajectory { unsafe { let code = xdrfile_xtc::write_xtc( self.handle.xdrfile, - to_i32(frame.len(), ErrorTask::Write)?, - to_i32(frame.step, ErrorTask::Write)?, + to_c_int(frame.len(), ErrorTask::Write)?, + to_c_int(frame.step, ErrorTask::Write)?, frame.time, frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], frame.coords[..].as_ptr() as *mut [f32; 3], @@ -317,12 +315,12 @@ impl Trajectory for XTCTrajectory { fn get_num_atoms(&mut self) -> Result { self.num_atoms .get_or_create(|| { - let mut num_atoms: i32 = 0; + let mut num_atoms: c_int = 0; 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); + let code = xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const c_int); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); @@ -384,8 +382,8 @@ impl TRRTrajectory { impl Trajectory for TRRTrajectory { fn read(&mut self, frame: &mut Frame) -> Result<()> { - let mut step: i32 = 0; - let mut lambda: f32 = 0.0; + let mut step: c_int = 0; + let mut lambda: c_float = 0.0; let num_atoms = self .get_num_atoms() @@ -395,13 +393,9 @@ impl Trajectory for TRRTrajectory { } unsafe { - // C lib requires an i32 to be passed, but step is exposed it as u32 - // (A step cannot be negative, can it?). So we need to create a step - // variable to pass to read_trr and cast it afterwards to u32. - // Similar for lambda. let code = xdrfile_trr::read_trr( self.handle.xdrfile, - to_i32(num_atoms, ErrorTask::Read)?, + to_c_int(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut lambda, @@ -422,8 +416,8 @@ impl Trajectory for TRRTrajectory { unsafe { let code = xdrfile_trr::write_trr( self.handle.xdrfile, - to_i32(frame.len(), ErrorTask::Write)?, - to_i32(frame.step, ErrorTask::Write)?, + to_c_int(frame.len(), ErrorTask::Write)?, + to_c_int(frame.step, ErrorTask::Write)?, frame.time, 0.0, frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], @@ -453,11 +447,11 @@ impl Trajectory for TRRTrajectory { fn get_num_atoms(&mut self) -> Result { self.num_atoms .get_or_create(|| { - let mut num_atoms: i32 = 0; + let mut num_atoms: c_int = 0; 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); + let code = xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const c_int); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); @@ -802,19 +796,22 @@ mod tests { } #[test] - fn test_to_i32() -> Result<()> { - assert_eq!(24234_i32, to_i32(24234_usize, ErrorTask::Read)?); + fn test_to_c_int() -> Result<()> { + assert_eq!(24234 as c_int, to_c_int(24234_usize, ErrorTask::Read)?); let try_from_int_err = match u8::try_from(-1) { Err(e) => e, _ => panic!("Conversion from -1 to u8 succeeded"), }; - let expected = Error::CastFromI32Failed { + let expected = Error::CastToCintFailed { source: try_from_int_err, task: ErrorTask::Write, value: 3_294_967_295_usize, }; - assert_eq!(Err(expected), to_i32(3_294_967_295_usize, ErrorTask::Write)); + assert_eq!( + Err(expected), + to_c_int(3_294_967_295_usize, ErrorTask::Write) + ); Ok(()) } From 9a4f27aeb72c9a730390b55f15631ebbaa8068a5 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 17:21:49 +1100 Subject: [PATCH 07/17] Removed pointer casts from lib.rs --- src/c_abi/xdrfile_xtc.rs | 4 ++-- src/lib.rs | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/c_abi/xdrfile_xtc.rs b/src/c_abi/xdrfile_xtc.rs index a002604..d59e8bf 100644 --- a/src/c_abi/xdrfile_xtc.rs +++ b/src/c_abi/xdrfile_xtc.rs @@ -29,8 +29,8 @@ extern "C" { natoms: ::std::os::raw::c_int, step: ::std::os::raw::c_int, time: ::std::os::raw::c_float, - box_vec: *mut Matrix, - x: *mut Rvec, + box_vec: *const Matrix, + x: *const Rvec, prec: ::std::os::raw::c_float, ) -> ::std::os::raw::c_int; } diff --git a/src/lib.rs b/src/lib.rs index d08332d..2bc12da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -289,8 +289,8 @@ impl Trajectory for XTCTrajectory { to_c_int(frame.len(), ErrorTask::Write)?, to_c_int(frame.step, ErrorTask::Write)?, frame.time, - frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], - frame.coords[..].as_ptr() as *mut [f32; 3], + &frame.box_vector, + frame.coords.as_ptr(), 1000.0, ); if let Some(err) = check_code(code, ErrorTask::Write) { @@ -320,7 +320,7 @@ 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 c_int); + let code = xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); @@ -420,8 +420,8 @@ impl Trajectory for TRRTrajectory { to_c_int(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], + &frame.box_vector, + frame.coords[..].as_ptr(), std::ptr::null_mut(), std::ptr::null_mut(), ); @@ -451,7 +451,7 @@ 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 c_int); + let code = xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms); // Reconstitute the CString so it is deallocated correctly let _ = CString::from_raw(path_p); From 4a71ed001c18b39b70033586a75d0571f5635990 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 17:39:22 +1100 Subject: [PATCH 08/17] New NumAtomsOutOfRange error variant --- src/errors.rs | 9 +++++++-- src/lib.rs | 6 ++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index c9d3982..a78dd2d 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -2,6 +2,7 @@ use crate::c_abi; use crate::FileMode; use crate::Frame; use std::error::Error as StdError; +use std::os::raw::c_int; use std::path::{Path, PathBuf}; /// Error type for the xdrfile library @@ -35,6 +36,8 @@ pub enum Error { task: ErrorTask, value: usize, }, + /// A numeric cast from `value` failed during `task` + NumAtomsOutOfRange(c_int), } impl Error { @@ -134,10 +137,12 @@ impl std::fmt::Display for Error { 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), + StepSizeOutOfRange(n) | NumAtomsOutOfRange(n) => { + write!(f, "Step {} does not fit in usize on this platform", n) + } CastToCintFailed { value, task, .. } => write!( f, - "Numeric cast from {value}:usize to i32 failed while {task}", + "Numeric cast from {value}:usize to C int failed while {task}", value = value, task = task ), diff --git a/src/lib.rs b/src/lib.rs index 2bc12da..a1f35a6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -327,8 +327,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(usize::try_from(num_atoms) - .expect("Number of atoms in file does not fit in usize")) + usize::try_from(num_atoms).map_err(|_| Error::NumAtomsOutOfRange(num_atoms)) } } }) @@ -458,8 +457,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(usize::try_from(num_atoms) - .expect("Number of atoms in file does not fit in usize")) + usize::try_from(num_atoms).map_err(|_| Error::NumAtomsOutOfRange(num_atoms)) } } }) From 2221531f758389b47420ddad7db2617315dc89ce Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 17:44:49 +1100 Subject: [PATCH 09/17] Improved out-of-range error messages --- src/errors.rs | 19 +++++++++++++------ src/lib.rs | 4 ++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index a78dd2d..74cdde8 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -29,15 +29,15 @@ pub enum Error { NullInStr(std::ffi::NulError), CouldNotCheckNAtoms(Box), /// Step was out of range for usize on this platform - StepSizeOutOfRange(i32), + StepOutOfRange(i32), + /// natoms was out of range for usize on this platform + NumAtomsOutOfRange(c_int), /// A numeric cast from `value` failed during `task` CastToCintFailed { source: std::num::TryFromIntError, task: ErrorTask, value: usize, }, - /// A numeric cast from `value` failed during `task` - NumAtomsOutOfRange(c_int), } impl Error { @@ -137,9 +137,16 @@ impl std::fmt::Display for Error { CouldNotCheckNAtoms(_err) => { write!(f, "Failed to read number of atoms in trajectory file") } - StepSizeOutOfRange(n) | NumAtomsOutOfRange(n) => { - write!(f, "Step {} does not fit in usize on this platform", n) - } + StepOutOfRange(n) => write!( + f, + "Illegal step size while reading trajectory: Failed to cast {} to usize.", + n + ), + NumAtomsOutOfRange(n) => write!( + f, + "Illegal number of atoms while reading trajectory: Failed to cast {} to usize.", + n + ), CastToCintFailed { value, task, .. } => write!( f, "Numeric cast from {value}:usize to C int failed while {task}", diff --git a/src/lib.rs b/src/lib.rs index a1f35a6..f8a5364 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -277,7 +277,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?; + frame.step = usize::try_from(step).map_err(|_| Error::StepOutOfRange(step))?; Ok(()) } } @@ -406,7 +406,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?; + frame.step = usize::try_from(step).map_err(|_| Error::StepOutOfRange(step))?; Ok(()) } } From 62462ed8c354e6f76963fb0bdb54633fc2a9281b Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 18:30:57 +1100 Subject: [PATCH 10/17] Unified conversion errors and improved messages --- src/errors.rs | 54 ++++++++++++++++------------------------------ src/lib.rs | 60 +++++++++++++++++++++++++++------------------------ 2 files changed, 51 insertions(+), 63 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 74cdde8..9e619d2 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -2,41 +2,29 @@ use crate::c_abi; use crate::FileMode; use crate::Frame; use std::error::Error as StdError; -use std::os::raw::c_int; use std::path::{Path, PathBuf}; /// Error type for the xdrfile library #[derive(Debug, Clone, PartialEq)] pub enum Error { /// An error code from the C API - CApiError { - code: ErrorCode, - task: ErrorTask, - }, + CApiError { code: ErrorCode, task: ErrorTask }, /// Passed in a frame of the wrong size - WrongSizeFrame { - expected: usize, - found: usize, - }, + WrongSizeFrame { expected: usize, found: usize }, /// C API failed to open a file (No return code provided) - CouldNotOpen { - path: PathBuf, - mode: FileMode, - }, + CouldNotOpen { path: PathBuf, mode: FileMode }, /// A path could not be converted to &OsStr InvalidOsStr, /// A path could not be converted to &CStr because it had a null byte NullInStr(std::ffi::NulError), + /// Checking the number of atoms failed while reading a frame CouldNotCheckNAtoms(Box), - /// Step was out of range for usize on this platform - StepOutOfRange(i32), - /// natoms was out of range for usize on this platform - NumAtomsOutOfRange(c_int), - /// A numeric cast from `value` failed during `task` - CastToCintFailed { - source: std::num::TryFromIntError, + /// Error for an out-of-range numeric conversion + OutOfRange { + name: &'static str, task: ErrorTask, - value: usize, + value: String, + target: &'static str, }, } @@ -75,7 +63,6 @@ impl std::error::Error for Error { match &self { NullInStr(err) => Some(err), CouldNotCheckNAtoms(err) => Some(err.as_ref()), - CastToCintFailed { source, .. } => Some(source), _ => None, } } @@ -137,21 +124,18 @@ impl std::fmt::Display for Error { CouldNotCheckNAtoms(_err) => { write!(f, "Failed to read number of atoms in trajectory file") } - StepOutOfRange(n) => write!( + OutOfRange { + name, + task, + value, + target, + } => write!( f, - "Illegal step size while reading trajectory: Failed to cast {} to usize.", - n - ), - NumAtomsOutOfRange(n) => write!( - f, - "Illegal number of atoms while reading trajectory: Failed to cast {} to usize.", - n - ), - CastToCintFailed { value, task, .. } => write!( - f, - "Numeric cast from {value}:usize to C int failed while {task}", + "Illegal {name} while {task}: Failed to cast {value} to {target}", + name = name, + task = task, value = value, - task = task + target = target ), } } diff --git a/src/lib.rs b/src/lib.rs index f8a5364..6376f3f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -109,14 +109,24 @@ fn path_to_cstring(path: impl AsRef) -> Result { Ok(CString::new(s)?) } -fn to_c_int(value: usize, task: ErrorTask) -> Result { - value.try_into().map_err(|e| Error::CastToCintFailed { - source: e, - value, +fn to(value: I, task: ErrorTask, name: &'static str) -> Result +where + I: TryInto + std::fmt::Display + Copy, +{ + value.try_into().map_err(|_| Error::OutOfRange { + name, + value: format!("{}", &value), + target: std::any::type_name::(), task, }) } +macro_rules! to { + ($value:expr, $task:expr) => { + to($value, $task, stringify!($value)) + }; +} + /// Convert an error code from a C call to an Error /// /// `code` should be an integer return code returned from the C API. @@ -267,7 +277,7 @@ impl Trajectory for XTCTrajectory { unsafe { let code = xdrfile_xtc::read_xtc( self.handle.xdrfile, - to_c_int(num_atoms, ErrorTask::Read)?, + to!(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut frame.box_vector, @@ -277,7 +287,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = usize::try_from(step).map_err(|_| Error::StepOutOfRange(step))?; + frame.step = to!(step, ErrorTask::ReadNumAtoms)?; Ok(()) } } @@ -286,8 +296,8 @@ impl Trajectory for XTCTrajectory { unsafe { let code = xdrfile_xtc::write_xtc( self.handle.xdrfile, - to_c_int(frame.len(), ErrorTask::Write)?, - to_c_int(frame.step, ErrorTask::Write)?, + to!(frame.num_atoms(), ErrorTask::Write)?, + to!(frame.step, ErrorTask::Write)?, frame.time, &frame.box_vector, frame.coords.as_ptr(), @@ -327,7 +337,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - usize::try_from(num_atoms).map_err(|_| Error::NumAtomsOutOfRange(num_atoms)) + to!(num_atoms, ErrorTask::ReadNumAtoms) } } }) @@ -394,7 +404,7 @@ impl Trajectory for TRRTrajectory { unsafe { let code = xdrfile_trr::read_trr( self.handle.xdrfile, - to_c_int(num_atoms, ErrorTask::Read)?, + to!(num_atoms, ErrorTask::Read)?, &mut step, &mut frame.time, &mut lambda, @@ -406,7 +416,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = usize::try_from(step).map_err(|_| Error::StepOutOfRange(step))?; + frame.step = to!(step, ErrorTask::ReadNumAtoms)?; Ok(()) } } @@ -415,8 +425,8 @@ impl Trajectory for TRRTrajectory { unsafe { let code = xdrfile_trr::write_trr( self.handle.xdrfile, - to_c_int(frame.len(), ErrorTask::Write)?, - to_c_int(frame.step, ErrorTask::Write)?, + to!(frame.len(), ErrorTask::Write)?, + to!(frame.step, ErrorTask::Write)?, frame.time, 0.0, &frame.box_vector, @@ -457,7 +467,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - usize::try_from(num_atoms).map_err(|_| Error::NumAtomsOutOfRange(num_atoms)) + to!(num_atoms, ErrorTask::ReadNumAtoms) } } }) @@ -794,22 +804,16 @@ mod tests { } #[test] - fn test_to_c_int() -> Result<()> { - assert_eq!(24234 as c_int, to_c_int(24234_usize, ErrorTask::Read)?); + fn test_to() -> Result<()> { + assert_eq!(24234_i32, to!(24234_usize, ErrorTask::Write)?); - let try_from_int_err = match u8::try_from(-1) { - Err(e) => e, - _ => panic!("Conversion from -1 to u8 succeeded"), - }; - let expected = Error::CastToCintFailed { - source: try_from_int_err, + let expected: Result = Err(Error::OutOfRange { + name: "3_294_967_295_usize", task: ErrorTask::Write, - value: 3_294_967_295_usize, - }; - assert_eq!( - Err(expected), - to_c_int(3_294_967_295_usize, ErrorTask::Write) - ); + value: "3294967295".to_string(), + target: "i32", + }); + assert_eq!(expected, to!(3_294_967_295_usize, ErrorTask::Write)); Ok(()) } From 43cc36de99103bfcb74abb1363d287e0a9524291 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 18:36:54 +1100 Subject: [PATCH 11/17] Corrected a few ErrorTasks --- src/lib.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 6376f3f..8708498 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -287,7 +287,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = to!(step, ErrorTask::ReadNumAtoms)?; + frame.step = to!(step, ErrorTask::Read)?; Ok(()) } } @@ -314,7 +314,7 @@ impl Trajectory for XTCTrajectory { fn flush(&mut self) -> Result<()> { unsafe { let code = xdr_seek::xdr_flush(self.handle.xdrfile); - if let Some(err) = check_code(code, ErrorTask::Read) { + if let Some(err) = check_code(code, ErrorTask::Flush) { Err(err) } else { Ok(()) @@ -416,7 +416,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::Read) { return Err(err); } - frame.step = to!(step, ErrorTask::ReadNumAtoms)?; + frame.step = to!(step, ErrorTask::Read)?; Ok(()) } } @@ -807,13 +807,14 @@ mod tests { fn test_to() -> Result<()> { assert_eq!(24234_i32, to!(24234_usize, ErrorTask::Write)?); + let big_number = 3_294_967_295_usize; let expected: Result = Err(Error::OutOfRange { - name: "3_294_967_295_usize", + name: "big_number", task: ErrorTask::Write, value: "3294967295".to_string(), target: "i32", }); - assert_eq!(expected, to!(3_294_967_295_usize, ErrorTask::Write)); + assert_eq!(expected, to!(big_number, ErrorTask::Write)); Ok(()) } From be6ac4202d8acfea1f116b289820ddd23c3e145e Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 15 Nov 2020 18:48:13 +1100 Subject: [PATCH 12/17] Test for the error messages from to! --- src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 8708498..7eb953b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -816,6 +816,13 @@ mod tests { }); assert_eq!(expected, to!(big_number, ErrorTask::Write)); + let num_atoms: usize = 304; + let res: Result = to!(num_atoms, ErrorTask::Write); + assert_eq!( + format!("{}", res.unwrap_err()), + "Illegal num_atoms while writing trajectory: Failed to cast 304 to u8" + ); + Ok(()) } } From ee7a266bb7656f3c479ca740560931395ce580ff Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 16 Nov 2020 11:50:40 +0100 Subject: [PATCH 13/17] single error type for cstring conversion --- src/errors.rs | 31 ++++++++++--------------------- src/lib.rs | 38 ++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 9e619d2..831e4b6 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -14,9 +14,7 @@ pub enum Error { /// C API failed to open a file (No return code provided) CouldNotOpen { path: PathBuf, mode: FileMode }, /// A path could not be converted to &OsStr - InvalidOsStr, - /// A path could not be converted to &CStr because it had a null byte - NullInStr(std::ffi::NulError), + InvalidOsStr(Option), /// Checking the number of atoms failed while reading a frame CouldNotCheckNAtoms(Box), /// Error for an out-of-range numeric conversion @@ -61,7 +59,13 @@ impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { use Error::*; match &self { - NullInStr(err) => Some(err), + InvalidOsStr(err) => { + if let Some(err) = err { + Some(err) + } else { + None + } + }, CouldNotCheckNAtoms(err) => Some(err.as_ref()), _ => None, } @@ -75,12 +79,6 @@ impl From<(ErrorCode, ErrorTask)> for Error { } } -impl From for Error { - fn from(err: std::ffi::NulError) -> Self { - Self::NullInStr(err) - } -} - impl From<(&Path, FileMode)> for Error { fn from(value: (&Path, FileMode)) -> Self { let (path, mode) = value; @@ -119,9 +117,8 @@ impl std::fmt::Display for Error { CouldNotOpen { path, mode } => { write!(f, "Could not open file at {:?} in mode {:?}", path, mode) } - InvalidOsStr => write!(f, "Paths must be valid unicode on this platform"), - NullInStr(_err) => write!(f, "Paths cannot include null bytes"), - CouldNotCheckNAtoms(_err) => { + InvalidOsStr(_) => write!(f, "Cannot convert path to CString."), + CouldNotCheckNAtoms(_) => { write!(f, "Failed to read number of atoms in trajectory file") } OutOfRange { @@ -249,8 +246,6 @@ pub type Result = std::result::Result; #[cfg(test)] mod tests { use super::*; - use std::ffi::CString; - use std::ffi::NulError; #[test] fn test_is_eof() { @@ -294,12 +289,6 @@ mod tests { #[test] fn test_from_correct_type() { - let nul_err: NulError = CString::new(b"foo\0".to_vec()).unwrap_err(); - let nul_err2: NulError = CString::new(b"foo\0".to_vec()).unwrap_err(); - let err = Error::from(nul_err); - let expected = Error::NullInStr(nul_err2); - assert_eq!(expected, err); - let code = 3.into(); let task = ErrorTask::Read; let expected = Error::CApiError { code, task }; diff --git a/src/lib.rs b/src/lib.rs index 7eb953b..ab1b25a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -105,8 +105,11 @@ impl FileMode { } fn path_to_cstring(path: impl AsRef) -> Result { - let s = path.as_ref().to_str().ok_or(Error::InvalidOsStr)?; - Ok(CString::new(s)?) + if let Some(s) = path.as_ref().to_str() { + CString::new(s).map_err(|e| Error::InvalidOsStr(Some(e))) + } else { + Err(Error::InvalidOsStr(None)) + } } fn to(value: I, task: ErrorTask, name: &'static str) -> Result @@ -621,21 +624,28 @@ mod tests { #[test] fn test_path_to_cstring() -> Result<(), Box> { - let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path")); - - if let Err(err) = result_invalid { - match err { - Error::NullInStr(_) => (), - Error::InvalidOsStr => (), - _ => panic!("Improper error type for path_to_cstring"), + // A valid string should convert to CString successfully + let valid_result = path_to_cstring(PathBuf::from("test")); + match valid_result { + Ok(s) => { + assert_eq!(s, CString::new("test")?); } - } else { - panic!("path_to_cstring should return Err if there are null bytes"); + Err(_) => panic!("Valid Path failed to convert to CString.") } - let result_valid = path_to_cstring("valid/path"); - assert_eq!(result_valid, Ok(CString::new("valid/path")?)); - + // \0 in path should result in an InvalidOsStr(Some(NulError)) + let result = path_to_cstring(PathBuf::from("invalid/\0path")); + match result { + Ok(_) => panic!("Cstring conversion did not fail"), + Err(e) => { + match e { + Error::InvalidOsStr(opt) => { + assert!(opt.is_some()) + } + _ => panic!("Wrong error type. (This should never happend).") + } + } + } Ok(()) } From 864ccf628ddaf7a1144e7fd9e14f684f5284f3d2 Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 16 Nov 2020 11:56:10 +0100 Subject: [PATCH 14/17] remove use statements in methods --- src/errors.rs | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 831e4b6..a5914e3 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -57,16 +57,15 @@ impl Error { impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; match &self { - InvalidOsStr(err) => { + Error::InvalidOsStr(err) => { if let Some(err) = err { Some(err) } else { None } }, - CouldNotCheckNAtoms(err) => Some(err.as_ref()), + Error::CouldNotCheckNAtoms(err) => Some(err.as_ref()), _ => None, } } @@ -101,27 +100,26 @@ 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 { - CApiError { code, task } => write!( + Error::CApiError { code, task } => write!( f, "Error while {task}: C API returned error code {code}", task = task, code = code ), - WrongSizeFrame { expected, found } => write!( + Error::WrongSizeFrame { expected, found } => write!( f, "Expected frame of size {:?}, found {:?}", expected, found ), - CouldNotOpen { path, mode } => { + Error::CouldNotOpen { path, mode } => { write!(f, "Could not open file at {:?} in mode {:?}", path, mode) } - InvalidOsStr(_) => write!(f, "Cannot convert path to CString."), - CouldNotCheckNAtoms(_) => { + Error::InvalidOsStr(_) => write!(f, "Cannot convert path to CString."), + Error::CouldNotCheckNAtoms(_) => { write!(f, "Failed to read number of atoms in trajectory file") } - OutOfRange { + Error::OutOfRange { name, task, value, @@ -155,13 +153,12 @@ pub enum ErrorTask { impl std::fmt::Display for ErrorTask { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - use ErrorTask::*; match &self { - ReadNumAtoms => write!(f, "reading atom number from trajectory"), - Read => write!(f, "reading trajectory"), - Write => write!(f, "writing trajectory"), - Flush => write!(f, "flushing trajectory"), - Seek => write!(f, "seeking in trajectory"), + ErrorTask::ReadNumAtoms => write!(f, "reading atom number from trajectory"), + ErrorTask::Read => write!(f, "reading trajectory"), + ErrorTask::Write => write!(f, "writing trajectory"), + ErrorTask::Flush => write!(f, "flushing trajectory"), + ErrorTask::Seek => write!(f, "seeking in trajectory"), } } } @@ -249,32 +246,31 @@ mod tests { #[test] fn test_is_eof() { - use Error::CApiError; - let error = CApiError { + let error = Error::CApiError { code: c_abi::xdrfile::exdrENDOFFILE.into(), task: ErrorTask::Read, }; assert!(error.is_eof()); - let error = CApiError { + let error = Error::CApiError { code: ErrorCode::ExdrEndOfFile, task: ErrorTask::Read, }; assert!(error.is_eof()); - let error = CApiError { + let error = Error::CApiError { code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(), task: ErrorTask::Read, }; assert!(!error.is_eof()); - let error = CApiError { + let error = Error::CApiError { code: 0.into(), task: ErrorTask::Read, }; assert!(!error.is_eof()); - let error = CApiError { + let error = Error::CApiError { code: 255.into(), task: ErrorTask::Read, }; From 46d358fd7aecd3fd4955981f2a9c035d581e2707 Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 16 Nov 2020 12:55:54 +0100 Subject: [PATCH 15/17] test outofrange error --- src/lib.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index ab1b25a..b0ec182 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -835,4 +835,33 @@ mod tests { Ok(()) } + + #[test] + fn test_write_outofrange_step() -> Result<(), Box> { + let tempfile = NamedTempFile::new()?; + let tmp_path = tempfile.path(); + let mut traj = XTCTrajectory::open_write(tmp_path)?; + + let frame = Frame { + step: usize::MAX, + time: 0.0, + box_vector: [[0.0; 3]; 3], + coords: vec![[1.0; 3]], + }; + let expected = Error::OutOfRange { + name: "frame.step", + value: usize::MAX.to_string(), + target: "i32", + task: ErrorTask::Write, + }; + + if let Err(e) = traj.write(&frame) { + print!("{:?}", e); + assert_eq!(expected, e); + } else { + panic!("Writing frame with step=usize::MAX should not succeed.") + } + + Ok(()) + } } From 40714061f6fe41c2d61592cfa34d68fd1f7ae077 Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 16 Nov 2020 13:07:08 +0100 Subject: [PATCH 16/17] check_code should not be public --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index b0ec182..d3331c2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -135,7 +135,7 @@ macro_rules! to { /// `code` should be an integer return code returned from the C API. /// If `code` indicates the function returned successfully, None is returned; /// otherwise, the code is converted into the appropriate `Error`. -pub fn check_code(code: impl Into, task: ErrorTask) -> Option { +fn check_code(code: impl Into, task: ErrorTask) -> Option { let code: ErrorCode = code.into(); if let ErrorCode::ExdrOk = code { None From 58617dcf4defc88c46f3fc821481b8f43f5d1116 Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 16 Nov 2020 13:26:03 +0100 Subject: [PATCH 17/17] documentation --- src/c_abi/mod.rs | 1 + src/iterator.rs | 9 ++++----- src/lib.rs | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/c_abi/mod.rs b/src/c_abi/mod.rs index 5bbb9e9..72b0a4b 100644 --- a/src/c_abi/mod.rs +++ b/src/c_abi/mod.rs @@ -1,3 +1,4 @@ +//! # Low level bindings to the c library from GROMACS #![allow(non_upper_case_globals, non_camel_case_types)] pub mod xdr_seek; diff --git a/src/iterator.rs b/src/iterator.rs index 5887ece..201dcf8 100644 --- a/src/iterator.rs +++ b/src/iterator.rs @@ -32,11 +32,10 @@ impl IntoIterator for TRRTrajectory { } } -/* -Iterator for trajectories. This iterator yields a Result -for each frame in the trajectory file and stops with yielding None once the -trajectory is finished. Also yields None after the first occurence of an error -*/ +/// Iterator for trajectories. +/// This iterator yields a Result for each frame in the +/// trajectory file and stops with yielding None once the trajectory is +/// EOF. Also yields None after the first occurrence of an error pub struct TrajectoryIterator { trajectory: T, item: Rc, diff --git a/src/lib.rs b/src/lib.rs index d3331c2..e296061 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -84,6 +84,7 @@ use std::io::SeekFrom; use std::os::raw::{c_float, c_int}; use std::path::{Path, PathBuf}; +/// File Mode for accessing trajectories. #[derive(Debug, Clone, PartialEq)] pub enum FileMode { Write, @@ -233,7 +234,7 @@ pub trait Trajectory { fn get_num_atoms(&mut self) -> Result; } -/// Read/Write XTC Trajectories +/// Handle to Read/Write XTC Trajectories pub struct XTCTrajectory { handle: XDRFile, precision: Cell, // internal mutability required for read method @@ -361,7 +362,7 @@ impl io::Seek for XTCTrajectory { } } -/// Read/Write TRR Trajectories +/// Handle to Read/Write TRR Trajectories pub struct TRRTrajectory { handle: XDRFile, num_atoms: Lazy>,