From f9cf479bafe89e189f62cbbde48667cdee419296 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Sun, 8 Nov 2020 17:23:08 +1100 Subject: [PATCH 1/3] Removed unwrap()s, refactored iterator, removed memleak --- README.md | 52 ++++++------- build.rs | 14 ++-- src/c_abi/xdr_seek.rs | 16 ++-- src/c_abi/xdrfile_trr.rs | 26 ++++--- src/c_abi/xdrfile_xtc.rs | 26 ++++--- src/iterator.rs | 107 +++++++++----------------- src/lib.rs | 158 ++++++++++++++++++++++++--------------- tests/integration.rs | 21 +++--- 8 files changed, 223 insertions(+), 197 deletions(-) diff --git a/README.md b/README.md index 3195c9b..de536c0 100644 --- a/README.md +++ b/README.md @@ -12,29 +12,27 @@ files with a safe api. ```rust use xdrfile::*; -// get a handle to the file -let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); +fn main() -> Result<()> { + // get a handle to the file + let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; -// find number of atoms in the file -let num_atoms = trj.get_num_atoms().unwrap(); + // find number of atoms in the file + let num_atoms = trj.get_num_atoms()?; -// a frame object is used to get to read or write from a trajectory -// without instantiating data arrays for every step -let mut frame = Frame::with_capacity(num_atoms); + // a frame object is used to get to read or write from a trajectory + // without instantiating data arrays for every step + let mut frame = Frame::with_capacity(num_atoms); -// read the first frame of the trajectory -let result = trj.read(&mut frame); -match result { - Ok(_) => { - assert_eq!(frame.step, 1); - assert_eq!(frame.num_atoms, num_atoms); + // read the first frame of the trajectory + trj.read(&mut frame)?; - let first_atom_coords = frame.coords[0]; - assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); - } - Err(msg) => { - panic!("Something went wrong: {}", msg); - } + assert_eq!(frame.step, 1); + assert_eq!(frame.num_atoms, num_atoms); + + let first_atom_coords = frame.coords[0]; + assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); + + Ok(()) } ``` @@ -47,13 +45,17 @@ Rc is required) ```rust use xdrfile::*; -// get a handle to the file -let trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); +fn main() -> Result<()> { + // get a handle to the file + let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; -// iterate over all frames -for (idx, frame) in trj.into_iter().filter_map(Result::ok).enumerate() { - println!("{}", frame.time); - assert_eq!(idx+1, frame.step as usize); + // iterate over all frames + for (idx, result) in trj.into_iter().enumerate() { + let frame = result?; + println!("{}", frame.time); + assert_eq!(idx+1, frame.step as usize); + } + Ok(()) } ``` diff --git a/build.rs b/build.rs index 59de77d..4603740 100644 --- a/build.rs +++ b/build.rs @@ -1,17 +1,17 @@ extern crate cc; use std::fs; +use std::io::Result; -fn main() { +fn main() -> Result<()> { // This builds gromacs' xdrfile library - let source_files: Vec<_> = fs::read_dir("external/xdrfile/src") - .unwrap() - .map(|f| f.unwrap()) - .map(|f| f.path()) - .collect(); + let source_files = fs::read_dir("external/xdrfile/src")? + .map(|r| r.map(|f| f.path())) + .collect::>>()?; cc::Build::new() .files(source_files) .include("external/xdrfile/include") .warnings(false) - .compile("libxdrfile.a") + .compile("libxdrfile.a"); + Ok(()) } diff --git a/src/c_abi/xdr_seek.rs b/src/c_abi/xdr_seek.rs index 457a08b..7e3482e 100644 --- a/src/c_abi/xdr_seek.rs +++ b/src/c_abi/xdr_seek.rs @@ -111,8 +111,8 @@ mod tests { use std::ffi::CString; #[test] - fn test_xdr_tell() { - let path = CString::new("tests/1l2y.xtc").unwrap(); + fn test_xdr_tell() -> Result<(), Box> { + let path = CString::new("tests/1l2y.xtc")?; let num_atoms = 304; let mut time: f32 = 2.0; let mut step: i32 = 5; @@ -121,7 +121,7 @@ mod tests { let mut prec: f32 = 0.0; unsafe { - let mode = CString::new("r").unwrap(); + let mode = CString::new("r")?; let xdr = xdrfile_open(path.as_ptr(), mode.as_ptr()); assert!(!xdr.is_null()); @@ -140,15 +140,16 @@ mod tests { let tell = xdr_tell(xdr); assert!(tell > 0, "{}", tell); - } + }; + Ok(()) } #[test] - fn test_xdr_seek() { - let path = CString::new("tests/1l2y.xtc").unwrap(); + fn test_xdr_seek() -> Result<(), Box> { + let path = CString::new("tests/1l2y.xtc")?; unsafe { - let mode = CString::new("r").unwrap(); + let mode = CString::new("r")?; let xdr = xdrfile_open(path.as_ptr(), mode.as_ptr()); assert!(!xdr.is_null()); @@ -160,5 +161,6 @@ mod tests { let tell = xdr_tell(xdr); assert!(tell == 500, "{}", tell); } + Ok(()) } } diff --git a/src/c_abi/xdrfile_trr.rs b/src/c_abi/xdrfile_trr.rs index d4078ff..751b356 100644 --- a/src/c_abi/xdrfile_trr.rs +++ b/src/c_abi/xdrfile_trr.rs @@ -47,19 +47,20 @@ mod tests { use tempfile::NamedTempFile; #[test] - fn test_read_trr_natoms() { - let path = CString::new("tests/1l2y.trr").unwrap(); + fn test_read_trr_natoms() -> Result<(), Box> { + let path = CString::new("tests/1l2y.trr")?; let mut natoms = 0; unsafe { read_trr_natoms(path.as_ptr() as *const i8, &mut natoms); } assert!(natoms == 304); + Ok(()) } #[test] - fn test_read_trr_nframes() { - let path = CString::new("tests/1l2y.trr").unwrap(); + fn test_read_trr_nframes() -> Result<(), Box> { + let path = CString::new("tests/1l2y.trr")?; let mut nframes: u64 = 0; unsafe { @@ -67,12 +68,18 @@ mod tests { assert!(code as u32 == exdrOK); } assert!(nframes == 38, "{:?}", nframes); + Ok(()) } #[test] - fn test_read_write_trr() { - let tempfile = NamedTempFile::new().unwrap(); - let tmp_path = CString::new(tempfile.path().to_str().unwrap()).unwrap(); + fn test_read_write_trr() -> Result<(), Box> { + let tempfile = NamedTempFile::new()?; + let tmp_path = CString::new( + tempfile + .path() + .to_str() + .expect("Could not convert path to str"), + )?; // write atoms to tempfile let natoms: i32 = 2; @@ -86,7 +93,7 @@ mod tests { let f: Vec = vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]]; unsafe { - let mode = CString::new("w").unwrap(); + let mode = CString::new("w")?; let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr()); let write_code = write_trr( xdr, @@ -114,7 +121,7 @@ mod tests { let f2: Vec = vec![[0.0, 0.0, 0.0]; 2]; unsafe { - let mode = CString::new("r").unwrap(); + let mode = CString::new("r")?; let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr()); let read_code = read_trr( xdr, @@ -139,5 +146,6 @@ mod tests { assert!(x2 == x); assert!(v2 == v); assert!(f2 == f); + Ok(()) } } diff --git a/src/c_abi/xdrfile_xtc.rs b/src/c_abi/xdrfile_xtc.rs index fda207f..507904f 100644 --- a/src/c_abi/xdrfile_xtc.rs +++ b/src/c_abi/xdrfile_xtc.rs @@ -43,19 +43,20 @@ mod tests { use tempfile::NamedTempFile; #[test] - fn test_read_xtc_natoms() { - let path = CString::new("tests/1l2y.xtc").unwrap(); + fn test_read_xtc_natoms() -> Result<(), Box> { + let path = CString::new("tests/1l2y.xtc")?; let mut natoms = 0; unsafe { read_xtc_natoms(path.as_ptr() as *mut i8, &mut natoms); } assert!(natoms == 304); + Ok(()) } #[test] - fn test_read_xtc_nframes() { - let path = CString::new("tests/1l2y.xtc").unwrap(); + fn test_read_xtc_nframes() -> Result<(), Box> { + let path = CString::new("tests/1l2y.xtc")?; let mut nframes: u64 = 0; unsafe { @@ -63,12 +64,18 @@ mod tests { assert!(code as u32 == exdrOK); } assert!(nframes == 38, "{:?}", nframes); + Ok(()) } #[test] - fn test_read_write_xtc() { - let tempfile = NamedTempFile::new().unwrap(); - let tmp_path = CString::new(tempfile.path().to_str().unwrap()).unwrap(); + fn test_read_write_xtc() -> Result<(), Box> { + let tempfile = NamedTempFile::new()?; + let tmp_path = CString::new( + tempfile + .path() + .to_str() + .expect("Could not convert path to str"), + )?; // write atoms to tempfile let natoms: i32 = 2; @@ -78,7 +85,7 @@ mod tests { let x: Vec = vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]]; unsafe { - let mode = CString::new("w").unwrap(); + let mode = CString::new("w")?; let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr()); let write_code = write_xtc( xdr, @@ -101,7 +108,7 @@ mod tests { let mut prec: f32 = 0.0; unsafe { - let mode = CString::new("r").unwrap(); + let mode = CString::new("r")?; let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr()); let read_code = read_xtc( xdr, @@ -121,5 +128,6 @@ mod tests { assert!(time2 == time); assert!(box_vec2 == box_vec); assert!(x2 == x); + Ok(()) } } diff --git a/src/iterator.rs b/src/iterator.rs index 9e25bc4..5f69fab 100644 --- a/src/iterator.rs +++ b/src/iterator.rs @@ -1,74 +1,35 @@ -use crate::c_abi::xdrfile::exdrENDOFFILE; use crate::*; use std::rc::Rc; impl IntoIterator for XTCTrajectory { type Item = Result>; - type IntoIter = XTCTrajectoryIterator; + type IntoIter = TrajectoryIterator; fn into_iter(mut self) -> Self::IntoIter { - // TODO this should be handled without the requirement of unwrap - let num_atoms = self.get_num_atoms().unwrap(); - XTCTrajectoryIterator { - trajectory: self, - item: Rc::new(Frame::with_capacity(num_atoms)), - has_error: false, - } - } -} - -/* -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 -*/ -pub struct XTCTrajectoryIterator { - trajectory: XTCTrajectory, - item: Rc, - has_error: bool, -} - -impl Iterator for XTCTrajectoryIterator { - type Item = Result>; - - fn next(&mut self) -> Option { - // Reuse old frame - if self.has_error { - return None; - } - - let item: &mut Frame = match Rc::get_mut(&mut self.item) { - Some(item) => item, - None => { - // caller kept frame. Create new one - self.item = Rc::new(Frame::with_capacity(self.item.num_atoms)); - Rc::get_mut(&mut self.item).unwrap() - } + let frame = match self.get_num_atoms() { + Ok(num_atoms) => Frame::with_capacity(num_atoms), + Err(_) => Frame::new(), }; - match self.trajectory.read(item) { - Ok(()) => Some(Ok(Rc::clone(&self.item))), - Err(msg) => { - if msg.to_string().contains(&exdrENDOFFILE.to_string()) { - None - } else { - self.has_error = true; - Some(Err(msg)) - } - } + TrajectoryIterator { + trajectory: self, + item: Rc::new(frame), + has_error: false, } } } impl IntoIterator for TRRTrajectory { type Item = Result>; - type IntoIter = TRRTrajectoryIterator; + type IntoIter = TrajectoryIterator; fn into_iter(mut self) -> Self::IntoIter { - // TODO this should be handled without the requirement of unwrap - let num_atoms = self.get_num_atoms().unwrap(); - TRRTrajectoryIterator { + let frame = match self.get_num_atoms() { + Ok(num_atoms) => Frame::with_capacity(num_atoms), + Err(_) => Frame::new(), + }; + TrajectoryIterator { trajectory: self, - item: Rc::new(Frame::with_capacity(num_atoms)), + item: Rc::new(frame), has_error: false, } } @@ -79,13 +40,16 @@ 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 */ -pub struct TRRTrajectoryIterator { - trajectory: TRRTrajectory, +pub struct TrajectoryIterator { + trajectory: T, item: Rc, has_error: bool, } -impl Iterator for TRRTrajectoryIterator { +impl Iterator for TrajectoryIterator +where + T: Trajectory, +{ type Item = Result>; fn next(&mut self) -> Option { @@ -99,18 +63,15 @@ impl Iterator for TRRTrajectoryIterator { None => { // caller kept frame. Create new one self.item = Rc::new(Frame::with_capacity(self.item.num_atoms)); - Rc::get_mut(&mut self.item).unwrap() + Rc::get_mut(&mut self.item).expect("Could not get mutable access to new Rc") } }; match self.trajectory.read(item) { Ok(()) => Some(Ok(Rc::clone(&self.item))), - Err(msg) => { - if msg.to_string().contains(&exdrENDOFFILE.to_string()) { - None - } else { - self.has_error = true; - Some(Err(msg)) - } + Err(e) if e.is_eof() => None, + Err(e) => { + self.has_error = true; + Some(Err(e)) } } } @@ -121,20 +82,24 @@ mod tests { use super::*; #[test] - pub fn test_xtc_trajectory_iterator() { - let traj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); - let frames: Vec> = traj.into_iter().filter_map(Result::ok).collect(); + pub fn test_xtc_trajectory_iterator() -> Result<()> { + let traj = XTCTrajectory::open_read("tests/1l2y.xtc")?; + let frames: Result>> = traj.into_iter().collect(); + let frames = frames?; assert!(frames.len() == 38); assert!(frames[0].step == 1, frames[0].step); assert!(frames[37].step == 38); + Ok(()) } #[test] - pub fn test_trr_trajectory_iterator() { - let traj = TRRTrajectory::open_read("tests/1l2y.trr").unwrap(); - let frames: Vec> = traj.into_iter().filter_map(Result::ok).collect(); + pub fn test_trr_trajectory_iterator() -> Result<()> { + let traj = TRRTrajectory::open_read("tests/1l2y.trr")?; + let frames: Result>> = traj.into_iter().collect(); + let frames = frames?; assert!(frames.len() == 38); assert!(frames[0].step == 1, frames[0].step); assert!(frames[37].step == 38); + Ok(()) } } diff --git a/src/lib.rs b/src/lib.rs index 68bff7c..771b3a6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,29 +9,27 @@ //! ```rust //! use xdrfile::*; //! -//! // get a handle to the file -//! let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); +//! fn main() -> Result<()> { +//! // get a handle to the file +//! let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; //! -//! // find number of atoms in the file -//! let num_atoms = trj.get_num_atoms().unwrap(); +//! // find number of atoms in the file +//! let num_atoms = trj.get_num_atoms()?; //! -//! // a frame object is used to get to read or write from a trajectory -//! // without instantiating data arrays for every step -//! let mut frame = Frame::with_capacity(num_atoms); +//! // a frame object is used to get to read or write from a trajectory +//! // without instantiating data arrays for every step +//! let mut frame = Frame::with_capacity(num_atoms); //! -//! // read the first frame of the trajectory -//! let result = trj.read(&mut frame); -//! match result { -//! Ok(_) => { -//! assert_eq!(frame.step, 1); -//! assert_eq!(frame.num_atoms, num_atoms); +//! // read the first frame of the trajectory +//! trj.read(&mut frame)?; //! -//! let first_atom_coords = frame.coords[0]; -//! assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); -//! } -//! Err(msg) => { -//! panic!("Something went wrong: {}", msg); -//! } +//! assert_eq!(frame.step, 1); +//! assert_eq!(frame.num_atoms, num_atoms); +//! +//! let first_atom_coords = frame.coords[0]; +//! assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); +//! +//! Ok(()) //! } //! ``` //! @@ -44,13 +42,17 @@ //! ```rust //! use xdrfile::*; //! -//! // get a handle to the file -//! let trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); +//! fn main() -> Result<()> { +//! // get a handle to the file +//! let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; //! -//! // iterate over all frames -//! for (idx, frame) in trj.into_iter().filter_map(Result::ok).enumerate() { -//! println!("{}", frame.time); -//! assert_eq!(idx+1, frame.step as usize); +//! // iterate over all frames +//! for (idx, result) in trj.into_iter().enumerate() { +//! let frame = result?; +//! println!("{}", frame.time); +//! assert_eq!(idx+1, frame.step as usize); +//! } +//! Ok(()) //! } //! ``` @@ -74,7 +76,7 @@ use c_abi::xdrfile_xtc; use lazy_init::Lazy; use std::cell::Cell; use std::ffi::CString; -use std::path::Path; +use std::path::{Path, PathBuf}; #[derive(Debug, Clone)] pub enum Error { @@ -83,6 +85,7 @@ pub enum Error { CouldNotRead(u32), CouldNotWrite(u32), CouldNotFlush(u32), + PathInvalidCstring, } impl std::fmt::Display for Error { @@ -115,13 +118,34 @@ impl std::fmt::Display for Error { "Failed to flush trajectory: C API returned error code {}", code ), + PathInvalidCstring => write!( + f, + "Path cannot be converted to a C string because it has a 0 byte" + ), + } + } +} + +impl Error { + pub fn is_eof(&self) -> bool { + use Error::*; + match self { + &CouldNotReadAtomNumber(code) + | &CouldNotRead(code) + | &CouldNotWrite(code) + | &CouldNotFlush(code) + if code == xdrfile::exdrENDOFFILE => + { + true + } + _ => false, } } } impl std::error::Error for Error {} -type Result = std::result::Result; +pub type Result = std::result::Result; #[derive(Debug, Clone, PartialEq)] pub enum FileMode { @@ -131,17 +155,21 @@ pub enum FileMode { } impl FileMode { - pub fn value(&self) -> &str { - match *self { - FileMode::Write => "w", - FileMode::Append => "a", - FileMode::Read => "r", - } + /// Get a CStr slice corresponding to the file mode + fn to_cstr(&self) -> &'static std::ffi::CStr { + let bytes: &[u8; 2] = match *self { + FileMode::Write => b"w\0", + FileMode::Append => b"a\0", + FileMode::Read => b"r\0", + }; + + std::ffi::CStr::from_bytes_with_nul(bytes).expect("CStr::from_bytes_with_nul failed") } } -fn path_to_cstring(path: &Path) -> CString { - CString::new(path.to_str().unwrap()).unwrap() +fn path_to_cstring(path: impl AsRef) -> Result { + let s = path.as_ref().to_str().ok_or(Error::PathInvalidCstring)?; + CString::new(s).map_err(|_| Error::PathInvalidCstring) } /// A safe wrapper around the c implementation of an XDRFile @@ -149,20 +177,24 @@ struct XDRFile { xdrfile: *mut XDRFILE, #[allow(dead_code)] filemode: FileMode, - path: String, + path: PathBuf, } impl XDRFile { pub fn open(path: impl AsRef, filemode: FileMode) -> Result { let path = path.as_ref(); - let path_p = path_to_cstring(path).into_raw(); - let mode_p = CString::new(filemode.value()).unwrap().into_raw(); - unsafe { + let path_p = path_to_cstring(path)?.into_raw(); + // SAFETY: mode_p must not be mutated by the C code + let mode_p = filemode.to_cstr().as_ptr(); + let xdrfile = xdrfile::xdrfile_open(path_p, mode_p); + // Reconstitute the CString so it is deallocated correctly + let _ = CString::from_raw(path_p); + if !xdrfile.is_null() { - let path = String::from(path.to_str().unwrap()); + let path = path.to_owned(); Ok(XDRFile { xdrfile, filemode, @@ -290,10 +322,12 @@ impl Trajectory for XTCTrajectory { .get_or_create(|| { let mut num_atoms: i32 = 0; unsafe { - let path = CString::new(self.handle.path.as_str()).unwrap(); + 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; + // Reconstitute the CString so it is deallocated correctly + let _ = CString::from_raw(path_p); match code { xdrfile::exdrOK => Ok(num_atoms as u32), _ => Err(Error::CouldNotReadAtomNumber(code)), @@ -398,10 +432,12 @@ impl Trajectory for TRRTrajectory { .get_or_create(|| { let mut num_atoms: i32 = 0; unsafe { - let path = CString::new(self.handle.path.as_str()).unwrap(); + 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; + // Reconstitute the CString so it is deallocated correctly + let _ = CString::from_raw(path_p); match code { xdrfile::exdrOK => Ok(num_atoms as u32), _ => Err(Error::CouldNotReadAtomNumber(code)), @@ -419,8 +455,8 @@ mod tests { use tempfile::NamedTempFile; #[test] - fn test_read_write_xtc() { - let tempfile = NamedTempFile::new().unwrap(); + fn test_read_write_xtc() -> Result<()> { + let tempfile = NamedTempFile::new().expect("Could not create temporary file"); let tmp_path = tempfile.path(); let natoms: u32 = 2; @@ -431,17 +467,17 @@ mod tests { box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], coords: vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]], }; - let mut f = XTCTrajectory::open_write(&tmp_path).unwrap(); + let mut f = XTCTrajectory::open_write(&tmp_path)?; let write_status = f.write(&frame); match write_status { Err(_) => panic!("Failed"), Ok(()) => {} } - f.flush().unwrap(); + f.flush()?; let mut new_frame = Frame::with_capacity(natoms); - let mut f = XTCTrajectory::open_read(tmp_path).unwrap(); - let num_atoms = f.get_num_atoms().unwrap(); + let mut f = XTCTrajectory::open_read(tmp_path)?; + let num_atoms = f.get_num_atoms()?; assert_eq!(num_atoms, natoms); let read_status = f.read(&mut new_frame); @@ -455,11 +491,12 @@ mod tests { assert_approx_eq!(new_frame.time, frame.time); assert_eq!(new_frame.box_vector, frame.box_vector); assert_eq!(new_frame.coords, frame.coords); + Ok(()) } #[test] - fn test_read_write_trr() { - let tempfile = NamedTempFile::new().unwrap(); + fn test_read_write_trr() -> Result<()> { + let tempfile = NamedTempFile::new().expect("Could not create temporary file"); let tmp_path = tempfile.path(); let natoms: u32 = 2; @@ -470,17 +507,17 @@ mod tests { box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], coords: vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]], }; - let mut f = TRRTrajectory::open_write(tmp_path).unwrap(); + let mut f = TRRTrajectory::open_write(tmp_path)?; let write_status = f.write(&frame); match write_status { Err(_) => panic!("Failed"), Ok(()) => {} } - f.flush().unwrap(); + f.flush()?; let mut new_frame = Frame::with_capacity(natoms); - let mut f = TRRTrajectory::open_read(tmp_path).unwrap(); - // let num_atoms = f.get_num_atoms().unwrap(); + let mut f = TRRTrajectory::open_read(tmp_path)?; + // let num_atoms = f.get_num_atoms()?; // assert_eq!(num_atoms, natoms); let read_status = f.read(&mut new_frame); @@ -494,6 +531,7 @@ mod tests { assert_eq!(new_frame.time, frame.time); assert_eq!(new_frame.box_vector, frame.box_vector); assert_eq!(new_frame.coords, frame.coords); + Ok(()) } #[test] @@ -512,9 +550,9 @@ mod tests { } #[test] - fn test_err_could_not_read_atom_nr() { + fn test_err_could_not_read_atom_nr() -> Result<()> { let file_name = "README.md"; // not a trajectory - let mut trr = TRRTrajectory::open_read(file_name).unwrap(); + let mut trr = TRRTrajectory::open_read(file_name)?; if let Err(e) = trr.get_num_atoms() { match e { Error::CouldNotReadAtomNumber(code) => { @@ -522,14 +560,15 @@ mod tests { } _ => panic!("Wrong Error type"), } - } + }; + Ok(()) } #[test] - fn test_err_could_not_read() { + fn test_err_could_not_read() -> Result<()> { let file_name = "README.md"; // not a trajectory let mut frame = Frame::with_capacity(1); - let mut trr = TRRTrajectory::open_read(file_name).unwrap(); + let mut trr = TRRTrajectory::open_read(file_name)?; if let Err(e) = trr.read(&mut frame) { match e { Error::CouldNotRead(code) => { @@ -538,5 +577,6 @@ mod tests { _ => panic!("Wrong Error type"), } } + Ok(()) } } diff --git a/tests/integration.rs b/tests/integration.rs index 2a2cf5b..f084289 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1,27 +1,28 @@ #[cfg(test)] mod integration { - use std::rc::Rc; use xdrfile::*; #[test] - fn test_use_library() { - let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); - let num_atoms = trj.get_num_atoms().unwrap(); + 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_capacity(num_atoms); - trj.read(&mut frame).unwrap(); - trj.read(&mut frame).unwrap(); + trj.read(&mut frame)?; + trj.read(&mut frame)?; assert_eq!(frame.step, 2); + Ok(()) } #[test] - fn test_use_library_iterator() { - let trj = XTCTrajectory::open_read("tests/1l2y.xtc").unwrap(); - let frames: Vec> = trj.into_iter().filter_map(Result::ok).collect(); - for (idx, frame) in frames.iter().enumerate() { + fn test_use_library_iterator() -> Result<()> { + 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); } + Ok(()) } } From e95fea3bfc5bec399ef554dcf343972cf2934c63 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Mon, 9 Nov 2020 17:33:03 +1100 Subject: [PATCH 2/3] Unit test for path_to_cstring() --- src/lib.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 771b3a6..0e8ec7e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -78,7 +78,7 @@ use std::cell::Cell; use std::ffi::CString; use std::path::{Path, PathBuf}; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub enum Error { CouldNotOpenFile(std::path::PathBuf, FileMode), CouldNotReadAtomNumber(u32), @@ -145,7 +145,7 @@ impl Error { impl std::error::Error for Error {} -pub type Result = std::result::Result; +pub type Result = std::result::Result; #[derive(Debug, Clone, PartialEq)] pub enum FileMode { @@ -579,4 +579,16 @@ mod tests { } Ok(()) } + + #[test] + fn test_path_to_cstring() -> Result<(), Box> { + let result_invalid = path_to_cstring("invalid/\0path"); + + assert_eq!(result_invalid, Err(Error::PathInvalidCstring)); + + let result_valid = path_to_cstring("valid/path"); + + assert_eq!(result_valid, Ok(CString::new("valid/path")?)); + Ok(()) + } } From fc35777aa03159995e09e8e9f1a980e1c44772f2 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Mon, 9 Nov 2020 18:06:08 +1100 Subject: [PATCH 3/3] Unit tests for Error::is_eof() --- src/lib.rs | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0e8ec7e..86ad06d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -534,6 +534,18 @@ mod tests { Ok(()) } + #[test] + fn test_path_to_cstring() -> Result<(), Box> { + let result_invalid = path_to_cstring("invalid/\0path"); + + assert_eq!(result_invalid, Err(Error::PathInvalidCstring)); + + let result_valid = path_to_cstring("valid/path"); + + assert_eq!(result_valid, Ok(CString::new("valid/path")?)); + Ok(()) + } + #[test] fn test_err_could_not_open() { let file_name = "non-existent.xtc"; @@ -581,14 +593,65 @@ mod tests { } #[test] - fn test_path_to_cstring() -> Result<(), Box> { - let result_invalid = path_to_cstring("invalid/\0path"); + fn test_err_eof() { + let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE); + assert!(error.is_eof()); - assert_eq!(result_invalid, Err(Error::PathInvalidCstring)); + let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE + 1); + assert!(!error.is_eof()); - let result_valid = path_to_cstring("valid/path"); + let error = Error::CouldNotWrite(0); + assert!(!error.is_eof()); + + let error = Error::CouldNotFlush(255); + assert!(!error.is_eof()); + + let error = Error::CouldNotOpenFile(PathBuf::from("not/a/file"), FileMode::Read); + assert!(!error.is_eof()); + } + + #[test] + fn test_err_file_eof() -> Result<(), Box> { + let tempfile = NamedTempFile::new()?; + let tmp_path = tempfile.path(); + + let natoms: u32 = 2; + let frame = Frame { + num_atoms: natoms, + step: 5, + time: 2.0, + box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], + coords: vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]], + }; + let mut f = XTCTrajectory::open_write(&tmp_path)?; + f.write(&frame)?; + f.flush()?; + + let mut new_frame = Frame::with_capacity(natoms); + let mut f = XTCTrajectory::open_read(tmp_path)?; + + f.read(&mut new_frame)?; + + let result = f.read(&mut new_frame); // Should be eof as we only wrote one frame + if let Err(e) = result { + assert!(e.is_eof()); + } else { + panic!("read two frames after writing one"); + } + + let mut file = std::fs::File::create(tmp_path)?; + use std::io::Write as _; + file.write_all(&[0; 999])?; + file.flush()?; + + let mut f = XTCTrajectory::open_read(tmp_path)?; + let result = f.read(&mut new_frame); // Should be an invalid XTC file + if let Err(e) = result { + assert!(!e.is_eof()); + } else { + panic!("999 zero bytes was read as a valid XTC file"); + } - assert_eq!(result_valid, Ok(CString::new("valid/path")?)); Ok(()) } }