From 55fa21d45c5b7178c206f8eadfa56f8bc4428d53 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sat, 7 Nov 2020 23:25:30 +1100
Subject: [PATCH 01/24] Broke out errors module and handle error codes as rust
 enum

---
 src/errors.rs | 220 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/lib.rs    | 184 +++++++++++++----------------------------
 2 files changed, 277 insertions(+), 127 deletions(-)
 create mode 100644 src/errors.rs

diff --git a/src/errors.rs b/src/errors.rs
new file mode 100644
index 0000000..efa09ac
--- /dev/null
+++ b/src/errors.rs
@@ -0,0 +1,220 @@
+use crate::c_abi;
+use crate::FileMode;
+use std::path::{Path, PathBuf};
+
+#[derive(Debug, Clone, PartialEq)]
+pub enum ErrorTask {
+    OpenFile(PathBuf, FileMode),
+    ReadNumAtoms,
+    Read,
+    Write,
+    Flush,
+    ToCString(Option<std::ffi::NulError>),
+}
+
+#[derive(Debug, Clone, PartialEq)]
+pub struct Error {
+    code: Option<ErrorCode>,
+    task: ErrorTask,
+}
+
+impl Error {
+    pub fn task(&self) -> &ErrorTask {
+        &self.task
+    }
+
+    pub fn code(&self) -> &Option<ErrorCode> {
+        &self.code
+    }
+
+    pub fn is_eof(&self) -> bool {
+        self.code.as_ref().map_or(false, ErrorCode::is_eof)
+    }
+
+    pub fn from_convert() -> Self {
+        Self {
+            code: None,
+            task: ErrorTask::ToCString(None),
+        }
+    }
+
+    pub fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
+        Self {
+            code: None,
+            task: ErrorTask::OpenFile(path.as_ref().into(), mode),
+        }
+    }
+
+    pub fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
+        Self {
+            code: Some(code.into()),
+            task: ErrorTask::ReadNumAtoms,
+        }
+    }
+
+    pub fn from_read(code: impl Into<ErrorCode>) -> Self {
+        Self {
+            code: Some(code.into()),
+            task: ErrorTask::Read,
+        }
+    }
+
+    pub fn from_write(code: impl Into<ErrorCode>) -> Self {
+        Self {
+            code: Some(code.into()),
+            task: ErrorTask::Write,
+        }
+    }
+
+    pub fn from_flush(code: impl Into<ErrorCode>) -> Self {
+        Self {
+            code: Some(code.into()),
+            task: ErrorTask::Flush,
+        }
+    }
+}
+
+impl From<std::ffi::NulError> for Error {
+    fn from(err: std::ffi::NulError) -> Self {
+        Self {
+            code: None,
+            task: ErrorTask::ToCString(Some(err)),
+        }
+    }
+}
+
+impl std::fmt::Display for Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorTask::*;
+        match &self.task {
+            OpenFile(path, mode) => write!(
+                f,
+                "Failed to open file at {path:?} with mode {mode:?}",
+                path = path,
+                mode = mode
+            ),
+            ReadNumAtoms => write!(
+                f,
+                "Failed to read atom number from trajectory: C API returned error code {:?}",
+                self.code
+            ),
+            Read => write!(
+                f,
+                "Failed to read trajectory: C API returned error code {:?}",
+                self.code
+            ),
+            Write => write!(
+                f,
+                "Failed to write trajectory: C API returned error code {:?}",
+                self.code
+            ),
+            Flush => write!(
+                f,
+                "Failed to flush trajectory: C API returned error code {:?}",
+                self.code
+            ),
+            ToCString(_) => write!(
+                f,
+                "Path cannot be converted to a C string because it has a null byte"
+            ),
+        }
+    }
+}
+
+#[derive(Debug, Clone, PartialEq)]
+pub enum ErrorCode {
+    ExdrOk,
+    ExdrHeader,
+    ExdrString,
+    ExdrDouble,
+    ExdrInt,
+    ExdrFloat,
+    ExdrUint,
+    Exdr3dx,
+    ExdrClose,
+    ExdrMagic,
+    ExdrNoMem,
+    ExdrEndOfFile,
+    ExdrFileNotFound,
+    ExdrNr,
+    UnmatchedCode(c_abi::xdrfile::BindgenTy1),
+}
+
+impl ErrorCode {
+    pub fn is_eof(&self) -> bool {
+        match self {
+            Self::ExdrEndOfFile => true,
+            _ => false,
+        }
+    }
+}
+
+impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
+    fn from(code: c_abi::xdrfile::BindgenTy1) -> Self {
+        match code {
+            c_abi::xdrfile::exdrOK => Self::ExdrOk,
+            c_abi::xdrfile::exdrHEADER => Self::ExdrHeader,
+            c_abi::xdrfile::exdrSTRING => Self::ExdrString,
+            c_abi::xdrfile::exdrDOUBLE => Self::ExdrDouble,
+            c_abi::xdrfile::exdrINT => Self::ExdrInt,
+            c_abi::xdrfile::exdrFLOAT => Self::ExdrFloat,
+            c_abi::xdrfile::exdrUINT => Self::ExdrUint,
+            c_abi::xdrfile::exdr3DX => Self::Exdr3dx,
+            c_abi::xdrfile::exdrCLOSE => Self::ExdrClose,
+            c_abi::xdrfile::exdrMAGIC => Self::ExdrMagic,
+            c_abi::xdrfile::exdrNOMEM => Self::ExdrNoMem,
+            c_abi::xdrfile::exdrENDOFFILE => Self::ExdrEndOfFile,
+            c_abi::xdrfile::exdrFILENOTFOUND => Self::ExdrFileNotFound,
+            c_abi::xdrfile::exdrNR => Self::ExdrNr,
+            code => Self::UnmatchedCode(code),
+        }
+    }
+}
+
+impl std::error::Error for Error {}
+
+pub type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_is_eof() {
+        let error = Error {
+            code: Some(c_abi::xdrfile::exdrENDOFFILE.into()),
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = Error {
+            code: Some(ErrorCode::ExdrEndOfFile),
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = Error {
+            code: Some((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error {
+            code: Some(0.into()),
+            task: ErrorTask::Write,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error {
+            code: Some(255.into()),
+            task: ErrorTask::Flush,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error {
+            code: None,
+            task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read),
+        };
+        assert!(!error.is_eof());
+    }
+}
diff --git a/src/lib.rs b/src/lib.rs
index 86ad06d..a8300d9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -62,8 +62,10 @@ extern crate assert_approx_eq;
 extern crate lazy_init;
 
 pub mod c_abi;
+mod errors;
 mod frame;
 mod iterator;
+pub use errors::*;
 pub use frame::Frame;
 pub use iterator::*;
 
@@ -78,75 +80,6 @@ use std::cell::Cell;
 use std::ffi::CString;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, PartialEq)]
-pub enum Error {
-    CouldNotOpenFile(std::path::PathBuf, FileMode),
-    CouldNotReadAtomNumber(u32),
-    CouldNotRead(u32),
-    CouldNotWrite(u32),
-    CouldNotFlush(u32),
-    PathInvalidCstring,
-}
-
-impl std::fmt::Display for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use Error::*;
-        match self {
-            CouldNotOpenFile(path, mode) => write!(
-                f,
-                "Failed to open file at {path:?} with mode {mode:?}",
-                path = path,
-                mode = mode
-            ),
-            CouldNotReadAtomNumber(code) => write!(
-                f,
-                "Failed to read atom number from trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotRead(code) => write!(
-                f,
-                "Failed to read trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotWrite(code) => write!(
-                f,
-                "Failed to write trajectory: C API returned error code {}",
-                code
-            ),
-            CouldNotFlush(code) => write!(
-                f,
-                "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 {}
-
-pub type Result<T, E = Error> = std::result::Result<T, E>;
-
 #[derive(Debug, Clone, PartialEq)]
 pub enum FileMode {
     Write,
@@ -168,8 +101,8 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    let s = path.as_ref().to_str().ok_or(Error::PathInvalidCstring)?;
-    CString::new(s).map_err(|_| Error::PathInvalidCstring)
+    let s = path.as_ref().to_str().ok_or_else(Error::from_convert)?;
+    Ok(CString::new(s)?)
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -202,7 +135,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::CouldNotOpenFile(path.into(), filemode))
+                Err(Error::from_open(path, filemode))
             }
         }
     }
@@ -282,9 +215,9 @@ impl Trajectory for XTCTrajectory {
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotRead(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_read(code)),
             }
         }
     }
@@ -300,9 +233,9 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotWrite(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_write(code)),
             }
         }
     }
@@ -310,9 +243,9 @@ impl Trajectory for XTCTrajectory {
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotFlush(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_flush(code)),
             }
         }
     }
@@ -328,9 +261,10 @@ impl Trajectory for XTCTrajectory {
                         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)),
+
+                    match code.into() {
+                        ErrorCode::ExdrOk => Ok(num_atoms as u32),
+                        _ => Err(Error::from_read_num_atoms(code)),
                     }
                 }
             })
@@ -390,9 +324,9 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotRead(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_read(code)),
             }
         }
     }
@@ -410,9 +344,9 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotWrite(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_write(code)),
             }
         }
     }
@@ -420,9 +354,9 @@ impl Trajectory for TRRTrajectory {
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code {
-                xdrfile::exdrOK => Ok(()),
-                _ => Err(Error::CouldNotFlush(code)),
+            match code.into() {
+                ErrorCode::ExdrOk => Ok(()),
+                _ => Err(Error::from_flush(code)),
             }
         }
     }
@@ -438,9 +372,10 @@ impl Trajectory for TRRTrajectory {
                         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)),
+
+                    match code.into() {
+                        ErrorCode::ExdrOk => Ok(num_atoms as u32),
+                        _ => Err(Error::from_read_num_atoms(code)),
                     }
                 }
             })
@@ -536,9 +471,21 @@ mod tests {
 
     #[test]
     fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
-        let result_invalid = path_to_cstring("invalid/\0path");
+        let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
 
-        assert_eq!(result_invalid, Err(Error::PathInvalidCstring));
+        assert_eq!(
+            result_invalid,
+            CString::new("invalid/\0path").map_err(Error::from)
+        );
+        assert!(!result_invalid.is_ok());
+        if let Err(e) = result_invalid {
+            match e.task() {
+                ErrorTask::ToCString(_) => (),
+                _ => panic!("path_to_cstring's errortask should be ErrorTask::ToCString(_)"),
+            }
+        } else {
+            panic!("path_to_cstring on a NULL-containing string should return an error");
+        }
 
         let result_valid = path_to_cstring("valid/path");
 
@@ -549,12 +496,13 @@ mod tests {
     #[test]
     fn test_err_could_not_open() {
         let file_name = "non-existent.xtc";
-        let expexted_path = Path::new(&file_name);
+
+        let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            match e {
-                Error::CouldNotOpenFile(err_path, err_mode) => {
-                    assert_eq!(expexted_path, err_path);
-                    assert!(FileMode::Read == err_mode)
+            match e.task() {
+                ErrorTask::OpenFile(err_path, err_mode) => {
+                    assert_eq!(path, err_path);
+                    assert_eq!(FileMode::Read, *err_mode)
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -566,9 +514,9 @@ mod tests {
         let file_name = "README.md"; // not a trajectory
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.get_num_atoms() {
-            match e {
-                Error::CouldNotReadAtomNumber(code) => {
-                    assert_eq!(xdrfile::exdrMAGIC, code);
+            match e.task() {
+                ErrorTask::ReadNumAtoms => {
+                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -582,9 +530,9 @@ mod tests {
         let mut frame = Frame::with_capacity(1);
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.read(&mut frame) {
-            match e {
-                Error::CouldNotRead(code) => {
-                    assert_eq!(xdrfile::exdrMAGIC, code);
+            match e.task() {
+                ErrorTask::Read => {
+                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -592,24 +540,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn test_err_eof() {
-        let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE);
-        assert!(error.is_eof());
-
-        let error = Error::CouldNotRead(xdrfile::exdrENDOFFILE + 1);
-        assert!(!error.is_eof());
-
-        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<dyn std::error::Error>> {
         let tempfile = NamedTempFile::new()?;

From 4ca96364b918fabe730350d286ad51480e5d765c Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sat, 7 Nov 2020 23:34:21 +1100
Subject: [PATCH 02/24] Made error construction methods pub(crate)

---
 src/errors.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index efa09ac..3e2eec3 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -31,42 +31,42 @@ impl Error {
         self.code.as_ref().map_or(false, ErrorCode::is_eof)
     }
 
-    pub fn from_convert() -> Self {
+    pub(crate) fn from_convert() -> Self {
         Self {
             code: None,
             task: ErrorTask::ToCString(None),
         }
     }
 
-    pub fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
+    pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
         Self {
             code: None,
             task: ErrorTask::OpenFile(path.as_ref().into(), mode),
         }
     }
 
-    pub fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
+    pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
             task: ErrorTask::ReadNumAtoms,
         }
     }
 
-    pub fn from_read(code: impl Into<ErrorCode>) -> Self {
+    pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
             task: ErrorTask::Read,
         }
     }
 
-    pub fn from_write(code: impl Into<ErrorCode>) -> Self {
+    pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
             task: ErrorTask::Write,
         }
     }
 
-    pub fn from_flush(code: impl Into<ErrorCode>) -> Self {
+    pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
             task: ErrorTask::Flush,

From 18436aaaa312ec4413c52c10040648d1394d9f57 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sat, 7 Nov 2020 23:45:14 +1100
Subject: [PATCH 03/24] Nicer error messages

---
 src/errors.rs | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 3e2eec3..0731bb3 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -86,41 +86,47 @@ impl From<std::ffi::NulError> for Error {
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use ErrorTask::*;
-        match &self.task {
-            OpenFile(path, mode) => write!(
+        match (&self.task, &self.code) {
+            (OpenFile(path, mode), _) => write!(
                 f,
                 "Failed to open file at {path:?} with mode {mode:?}",
                 path = path,
                 mode = mode
             ),
-            ReadNumAtoms => write!(
+            (ReadNumAtoms, Some(code)) => write!(
                 f,
-                "Failed to read atom number from trajectory: C API returned error code {:?}",
-                self.code
+                "Failed to read atom number from trajectory: C API returned error code {}",
+                code
             ),
-            Read => write!(
+            (ReadNumAtoms, None) => write!(f, "Failed to read atom number from trajectory"),
+            (Read, Some(code)) => write!(
                 f,
-                "Failed to read trajectory: C API returned error code {:?}",
-                self.code
+                "Failed to read trajectory: C API returned error code {}",
+                code
             ),
-            Write => write!(
+            (Read, None) => write!(f, "Failed to read trajectory"),
+            (Write, Some(code)) => write!(
                 f,
-                "Failed to write trajectory: C API returned error code {:?}",
-                self.code
+                "Failed to write trajectory: C API returned error code {}",
+                code
             ),
-            Flush => write!(
+            (Write, None) => write!(f, "Failed to write trajectory"),
+            (Flush, Some(code)) => write!(
                 f,
-                "Failed to flush trajectory: C API returned error code {:?}",
-                self.code
+                "Failed to flush trajectory: C API returned error code {}",
+                code
             ),
-            ToCString(_) => write!(
+            (ToCString(_), _) => write!(
                 f,
                 "Path cannot be converted to a C string because it has a null byte"
             ),
+            (Flush, None) => write!(f, "Failed to flush trajectory"),
         }
     }
 }
 
+impl std::error::Error for Error {}
+
 #[derive(Debug, Clone, PartialEq)]
 pub enum ErrorCode {
     ExdrOk,
@@ -171,7 +177,15 @@ impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
     }
 }
 
-impl std::error::Error for Error {}
+impl std::fmt::Display for ErrorCode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Self::UnmatchedCode(i) = self {
+            write!(f, "{}", i)
+        } else {
+            write!(f, "{:?}", self)
+        }
+    }
+}
 
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 

From 36a722e721785737a49690efe0b46afc48f1bd15 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sat, 7 Nov 2020 23:57:12 +1100
Subject: [PATCH 04/24] Refactored ErrorCode as Error.source()

---
 src/errors.rs | 49 +++++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 0731bb3..42d78e3 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -86,46 +86,34 @@ impl From<std::ffi::NulError> for Error {
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use ErrorTask::*;
-        match (&self.task, &self.code) {
-            (OpenFile(path, mode), _) => write!(
+        match &self.task {
+            OpenFile(path, mode) => write!(
                 f,
                 "Failed to open file at {path:?} with mode {mode:?}",
                 path = path,
                 mode = mode
             ),
-            (ReadNumAtoms, Some(code)) => write!(
-                f,
-                "Failed to read atom number from trajectory: C API returned error code {}",
-                code
-            ),
-            (ReadNumAtoms, None) => write!(f, "Failed to read atom number from trajectory"),
-            (Read, Some(code)) => write!(
-                f,
-                "Failed to read trajectory: C API returned error code {}",
-                code
-            ),
-            (Read, None) => write!(f, "Failed to read trajectory"),
-            (Write, Some(code)) => write!(
-                f,
-                "Failed to write trajectory: C API returned error code {}",
-                code
-            ),
-            (Write, None) => write!(f, "Failed to write trajectory"),
-            (Flush, Some(code)) => write!(
-                f,
-                "Failed to flush trajectory: C API returned error code {}",
-                code
-            ),
-            (ToCString(_), _) => write!(
+            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
+            Read => write!(f, "Failed to read trajectory"),
+            Write => write!(f, "Failed to write trajectory"),
+            Flush => write!(f, "Failed to flush trajectory"),
+            ToCString(_) => write!(
                 f,
                 "Path cannot be converted to a C string because it has a null byte"
             ),
-            (Flush, None) => write!(f, "Failed to flush trajectory"),
         }
     }
 }
 
-impl std::error::Error for Error {}
+impl std::error::Error for Error {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        if let Some(e) = &self.code {
+            Some(e)
+        } else {
+            None
+        }
+    }
+}
 
 #[derive(Debug, Clone, PartialEq)]
 pub enum ErrorCode {
@@ -180,13 +168,14 @@ impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
 impl std::fmt::Display for ErrorCode {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         if let Self::UnmatchedCode(i) = self {
-            write!(f, "{}", i)
+            write!(f, "C API returned error code {}", i)
         } else {
-            write!(f, "{:?}", self)
+            write!(f, "C API returned error code {:?}", self)
         }
     }
 }
 
+impl std::error::Error for ErrorCode {}
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
 #[cfg(test)]

From 95b1bdba57f9974df7025fc5d6bab588bad2a9d1 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sun, 8 Nov 2020 00:17:37 +1100
Subject: [PATCH 05/24] Streamlined error generation

---
 src/errors.rs | 11 ++++++++++-
 src/lib.rs    | 47 ++++++++++++-----------------------------------
 2 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 42d78e3..b08d09b 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -115,7 +115,7 @@ impl std::error::Error for Error {
     }
 }
 
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq, Copy)]
 pub enum ErrorCode {
     ExdrOk,
     ExdrHeader,
@@ -141,6 +141,15 @@ impl ErrorCode {
             _ => false,
         }
     }
+
+    pub fn check<T>(code: impl Into<Self>, value: T) -> std::result::Result<T, Self> {
+        let code = code.into();
+        if let Self::ExdrOk = code {
+            Ok(value)
+        } else {
+            Err(code)
+        }
+    }
 }
 
 impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
diff --git a/src/lib.rs b/src/lib.rs
index a8300d9..ee5564f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -200,11 +200,11 @@ impl XTCTrajectory {
 
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
+        let mut step: i32 = 0;
         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 mut step: i32 = 0;
             let code = xdrfile_xtc::read_xtc(
                 self.handle.xdrfile,
                 frame.num_atoms as i32,
@@ -215,10 +215,7 @@ impl Trajectory for XTCTrajectory {
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_read(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_read)
         }
     }
 
@@ -233,20 +230,14 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_write(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_flush(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_flush)
         }
     }
 
@@ -254,6 +245,7 @@ impl Trajectory for XTCTrajectory {
         self.num_atoms
             .get_or_create(|| {
                 let mut num_atoms: i32 = 0;
+
                 unsafe {
                     let path = path_to_cstring(&self.handle.path)?;
                     let path_p = path.into_raw();
@@ -262,10 +254,7 @@ impl Trajectory for XTCTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    match code.into() {
-                        ErrorCode::ExdrOk => Ok(num_atoms as u32),
-                        _ => Err(Error::from_read_num_atoms(code)),
-                    }
+                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
                 }
             })
             .clone()
@@ -305,13 +294,13 @@ 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;
         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 mut step: i32 = 0;
-            let mut lambda: f32 = 0.0;
             let code = xdrfile_trr::read_trr(
                 self.handle.xdrfile,
                 frame.num_atoms as i32,
@@ -324,10 +313,7 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_read(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_read)
         }
     }
 
@@ -344,20 +330,14 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_write(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            match code.into() {
-                ErrorCode::ExdrOk => Ok(()),
-                _ => Err(Error::from_flush(code)),
-            }
+            ErrorCode::check(code, ()).map_err(Error::from_flush)
         }
     }
 
@@ -373,10 +353,7 @@ impl Trajectory for TRRTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    match code.into() {
-                        ErrorCode::ExdrOk => Ok(num_atoms as u32),
-                        _ => Err(Error::from_read_num_atoms(code)),
-                    }
+                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
                 }
             })
             .clone()

From f75707252313c5e704ade77b6a6ca9adb74f973b Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Sun, 8 Nov 2020 01:19:52 +1100
Subject: [PATCH 06/24] Added documentation to the errors module

---
 src/errors.rs | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/src/errors.rs b/src/errors.rs
index b08d09b..e6c5ebd 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -3,34 +3,46 @@ use crate::FileMode;
 use std::path::{Path, PathBuf};
 
 #[derive(Debug, Clone, PartialEq)]
+/// The task being attempted when an error was returned
 pub enum ErrorTask {
+    /// A file was being opened
     OpenFile(PathBuf, FileMode),
+    /// The number of atoms was being read from a file
     ReadNumAtoms,
+    /// A frame was being read from a file
     Read,
+    /// A frame was being written to a file
     Write,
+    /// An file was being flushed to disk
     Flush,
+    /// A path was being converted to a CString
     ToCString(Option<std::ffi::NulError>),
 }
 
 #[derive(Debug, Clone, PartialEq)]
+/// Error type for the xdrfile library
 pub struct Error {
     code: Option<ErrorCode>,
     task: ErrorTask,
 }
 
 impl Error {
+    /// Get the task being attempted when the error was returned
     pub fn task(&self) -> &ErrorTask {
         &self.task
     }
 
+    /// Get the error code returned by the C API, if any
     pub fn code(&self) -> &Option<ErrorCode> {
         &self.code
     }
 
+    /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
         self.code.as_ref().map_or(false, ErrorCode::is_eof)
     }
 
+    /// Construct a new error during the ToCString task
     pub(crate) fn from_convert() -> Self {
         Self {
             code: None,
@@ -38,6 +50,7 @@ impl Error {
         }
     }
 
+    /// Construct a new error during the OpenFile task
     pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
         Self {
             code: None,
@@ -45,6 +58,7 @@ impl Error {
         }
     }
 
+    /// Construct a new error during the ReadNumAtoms task from a C error code
     pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
@@ -52,6 +66,7 @@ impl Error {
         }
     }
 
+    /// Construct a new error during the Read task from a C error code
     pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
@@ -59,6 +74,7 @@ impl Error {
         }
     }
 
+    /// Construct a new error during the Write task from a C error code
     pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
@@ -66,6 +82,7 @@ impl Error {
         }
     }
 
+    /// Construct a new error during the Flush task from a C error code
     pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
         Self {
             code: Some(code.into()),
@@ -116,25 +133,42 @@ impl std::error::Error for Error {
 }
 
 #[derive(Debug, Clone, PartialEq, Copy)]
+/// Error codes returned from the C API
 pub enum ErrorCode {
+    /// No error, C API returned successfully
     ExdrOk,
+    /// TRR file had corrupt Header
     ExdrHeader,
+    /// Failed to read a string where expected
     ExdrString,
+    /// Failed to read a double where expected
     ExdrDouble,
+    /// Failed to read an int where expected
     ExdrInt,
+    /// Failed to read a float where expected
     ExdrFloat,
+    /// Failed to read a uint where expected
     ExdrUint,
+    /// Failed to read compressed XTC coordinates
     Exdr3dx,
+    /// Error encountered while closing file
     ExdrClose,
+    /// File had incorrect "magic number" (not an XTC file)
     ExdrMagic,
+    /// Failed to allocate memory for TRR file
     ExdrNoMem,
+    /// End of file was reached while trying to read
     ExdrEndOfFile,
+    /// File was not found when trying to open
     ExdrFileNotFound,
+    /// Failed to seek within file
     ExdrNr,
+    /// Something unexpected happened
     UnmatchedCode(c_abi::xdrfile::BindgenTy1),
 }
 
 impl ErrorCode {
+    /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
         match self {
             Self::ExdrEndOfFile => true,
@@ -142,6 +176,12 @@ impl ErrorCode {
         }
     }
 
+    /// Convert an error code and output value from a C call to a Result
+    ///
+    /// `code` should be an integer return code returned from the C API. `value` should be the
+    /// function's output, which is generally either `()` or one of its arguments. If `code`
+    /// indicates the function returned successfully, the value is returned; otherwise, the
+    /// code is converted into the appropriate `ErrorCode`.
     pub fn check<T>(code: impl Into<Self>, value: T) -> std::result::Result<T, Self> {
         let code = code.into();
         if let Self::ExdrOk = code {
@@ -185,6 +225,8 @@ impl std::fmt::Display for ErrorCode {
 }
 
 impl std::error::Error for ErrorCode {}
+
+/// `Result` type for errors in the `xdrfile` crate
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
 #[cfg(test)]

From 9ac3c959d9271166631bb5ce0a6084ba3f56e681 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Mon, 9 Nov 2020 22:40:37 +1100
Subject: [PATCH 07/24] Added ToCString variant to Error.source()

---
 src/errors.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/errors.rs b/src/errors.rs
index e6c5ebd..905467d 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -126,6 +126,8 @@ impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         if let Some(e) = &self.code {
             Some(e)
+        } else if let ErrorTask::ToCString(Some(e)) = &self.task {
+            Some(e)
         } else {
             None
         }

From d1b69d3fd467833af0acb8784588603c7efc386f Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Mon, 9 Nov 2020 22:42:50 +1100
Subject: [PATCH 08/24] Treat ToCString differently in Display based on whether
 it has a source

---
 src/errors.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/errors.rs b/src/errors.rs
index 905467d..20f57d2 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -114,7 +114,11 @@ impl std::fmt::Display for Error {
             Read => write!(f, "Failed to read trajectory"),
             Write => write!(f, "Failed to write trajectory"),
             Flush => write!(f, "Failed to flush trajectory"),
-            ToCString(_) => write!(
+            ToCString(None) => write!(
+                f,
+                "Path cannot be converted to a C string because it was invalid Unicode"
+            ),
+            ToCString(Some(_)) => write!(
                 f,
                 "Path cannot be converted to a C string because it has a null byte"
             ),

From 1fbce21ff6ff8d06938df28d77cef2ed652a7e14 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 00:59:10 +1100
Subject: [PATCH 09/24] Corrected undefined behaviour when calling
 Trajectory.read() on improperly sized Frame

---
 src/errors.rs | 63 ++++++++++++++++++++++++++++++++++++---------------
 src/frame.rs  | 12 +++++++---
 src/lib.rs    | 44 +++++++++++++++++++++++++++++++++--
 3 files changed, 96 insertions(+), 23 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 20f57d2..f803a0f 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -22,8 +22,9 @@ pub enum ErrorTask {
 #[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
 pub struct Error {
-    code: Option<ErrorCode>,
+    code: ErrorCode,
     task: ErrorTask,
+    source: Option<Box<Error>>,
 }
 
 impl Error {
@@ -33,60 +34,75 @@ impl Error {
     }
 
     /// Get the error code returned by the C API, if any
-    pub fn code(&self) -> &Option<ErrorCode> {
+    pub fn code(&self) -> &ErrorCode {
         &self.code
     }
 
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        self.code.as_ref().map_or(false, ErrorCode::is_eof)
+        self.code.is_eof()
     }
 
     /// Construct a new error during the ToCString task
     pub(crate) fn from_convert() -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::ToCString(None),
+            source: None,
         }
     }
 
     /// Construct a new error during the OpenFile task
     pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::OpenFile(path.as_ref().into(), mode),
+            source: None,
         }
     }
 
     /// Construct a new error during the ReadNumAtoms task from a C error code
     pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::ReadNumAtoms,
+            source: None,
         }
     }
 
     /// Construct a new error during the Read task from a C error code
     pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Read,
+            source: None,
         }
     }
 
     /// Construct a new error during the Write task from a C error code
     pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Write,
+            source: None,
         }
     }
 
     /// Construct a new error during the Flush task from a C error code
     pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Flush,
+            source: None,
+        }
+    }
+
+    /// Construct a new error during the ReadNumAtoms task from a C error code
+    pub(crate) fn with_task(self, task: ErrorTask) -> Self {
+        Self {
+            code: self.code,
+            task,
+            source: Some(Box::new(self)),
         }
     }
 }
@@ -94,8 +110,9 @@ impl Error {
 impl From<std::ffi::NulError> for Error {
     fn from(err: std::ffi::NulError) -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::ToCString(Some(err)),
+            source: None,
         }
     }
 }
@@ -128,8 +145,10 @@ impl std::fmt::Display for Error {
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        if let Some(e) = &self.code {
-            Some(e)
+        if let Some(source) = &self.source {
+            Some(source.as_ref())
+        } else if self.code != ErrorCode::NoCode {
+            Some(&self.code)
         } else if let ErrorTask::ToCString(Some(e)) = &self.task {
             Some(e)
         } else {
@@ -171,6 +190,8 @@ pub enum ErrorCode {
     ExdrNr,
     /// Something unexpected happened
     UnmatchedCode(c_abi::xdrfile::BindgenTy1),
+    /// C returned no code at all
+    NoCode,
 }
 
 impl ErrorCode {
@@ -242,38 +263,44 @@ mod tests {
     #[test]
     fn test_is_eof() {
         let error = Error {
-            code: Some(c_abi::xdrfile::exdrENDOFFILE.into()),
+            code: c_abi::xdrfile::exdrENDOFFILE.into(),
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: Some(ErrorCode::ExdrEndOfFile),
+            code: ErrorCode::ExdrEndOfFile,
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: Some((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
+            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: Some(0.into()),
+            code: 0.into(),
             task: ErrorTask::Write,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: Some(255.into()),
+            code: 255.into(),
             task: ErrorTask::Flush,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read),
+            source: None,
         };
         assert!(!error.is_eof());
     }
diff --git a/src/frame.rs b/src/frame.rs
index 805f8e7..d8e2650 100644
--- a/src/frame.rs
+++ b/src/frame.rs
@@ -1,7 +1,7 @@
 use std::fmt;
 
 /// A frame represents a single step in a trajectory.
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct Frame {
     /// Number of atoms in the frame
     pub num_atoms: u32,
@@ -13,10 +13,10 @@ pub struct Frame {
     pub time: f32,
 
     /// 3x3 box vector
-    pub box_vector: [[f32; 3usize]; 3usize],
+    pub box_vector: [[f32; 3]; 3],
 
     /// 3D coordinates for N atoms where N is num_atoms
-    pub coords: Vec<[f32; 3usize]>,
+    pub coords: Vec<[f32; 3]>,
 }
 
 impl Default for Frame {
@@ -76,6 +76,12 @@ impl Frame {
     pub fn len(self: &Frame) -> usize {
         self.num_atoms as usize
     }
+
+    /// Resizes the Frame in-place so that num_atoms is equal to new_size.
+    pub fn resize(&mut self, new_size: u32) {
+        self.num_atoms = new_size;
+        self.coords.resize(new_size as usize, [0.0; 3]);
+    }
 }
 
 #[cfg(test)]
diff --git a/src/lib.rs b/src/lib.rs
index ee5564f..9502d43 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -201,6 +201,10 @@ impl XTCTrajectory {
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
+        frame.resize(
+            self.get_num_atoms()
+                .map_err(|e| e.with_task(ErrorTask::Read))?,
+        );
         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
@@ -296,6 +300,10 @@ impl Trajectory for TRRTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
+        frame.resize(
+            self.get_num_atoms()
+                .map_err(|e| e.with_task(ErrorTask::Read))?,
+        );
         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
@@ -446,6 +454,38 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    pub fn test_manual_loop() -> Result<(), Box<dyn std::error::Error>> {
+        let mut frame = Frame::new();
+
+        let mut xtc_frames = Vec::new();
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+
+        while let Ok(()) = xtc_traj.read(&mut frame) {
+            xtc_frames.push(frame.clone());
+        }
+
+        let mut trr_frames = Vec::new();
+        let mut trr_traj = TRRTrajectory::open_read("tests/1l2y.trr")?;
+
+        while let Ok(()) = trr_traj.read(&mut frame) {
+            trr_frames.push(frame.clone());
+        }
+
+        for (xtc, trr) in xtc_frames.into_iter().zip(trr_frames) {
+            assert_eq!(xtc.num_atoms, trr.num_atoms);
+            assert_eq!(xtc.step, trr.step);
+            assert_eq!(xtc.time, trr.time);
+            assert_eq!(xtc.box_vector, trr.box_vector);
+            for (xtc_xyz, trr_xyz) in xtc.coords.into_iter().zip(trr.coords) {
+                assert!(xtc_xyz[0] - trr_xyz[0] <= 1e-5);
+                assert!(xtc_xyz[1] - trr_xyz[1] <= 1e-5);
+                assert!(xtc_xyz[2] - trr_xyz[2] <= 1e-5);
+            }
+        }
+        Ok(())
+    }
+
     #[test]
     fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
@@ -493,7 +533,7 @@ mod tests {
         if let Err(e) = trr.get_num_atoms() {
             match e.task() {
                 ErrorTask::ReadNumAtoms => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
+                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -509,7 +549,7 @@ mod tests {
         if let Err(e) = trr.read(&mut frame) {
             match e.task() {
                 ErrorTask::Read => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
+                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }

From 65294abee878b19052b70c0658da90f4c7e4f591 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 17:41:11 +1100
Subject: [PATCH 10/24] Replaced frame resizing with a simple check

---
 src/errors.rs | 269 +++++++++++++++++++++++++++-----------------------
 src/frame.rs  |  12 +--
 src/lib.rs    | 119 +++++++++++++---------
 3 files changed, 224 insertions(+), 176 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index f803a0f..2115bb3 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -1,12 +1,13 @@
 use crate::c_abi;
 use crate::FileMode;
+use crate::Frame;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Copy, PartialEq)]
 /// The task being attempted when an error was returned
 pub enum ErrorTask {
     /// A file was being opened
-    OpenFile(PathBuf, FileMode),
+    OpenFile,
     /// The number of atoms was being read from a file
     ReadNumAtoms,
     /// A frame was being read from a file
@@ -16,13 +17,30 @@ pub enum ErrorTask {
     /// An file was being flushed to disk
     Flush,
     /// A path was being converted to a CString
-    ToCString(Option<std::ffi::NulError>),
+    ToCString,
+    /// Unknown task
+    UnknownTask,
+}
+
+impl std::fmt::Display for ErrorTask {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorTask::*;
+        match &self {
+            OpenFile => write!(f, "Failed to open file as trajectory"),
+            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
+            Read => write!(f, "Failed to read trajectory"),
+            Write => write!(f, "Failed to write trajectory"),
+            Flush => write!(f, "Failed to flush trajectory"),
+            ToCString => write!(f, "Failed to convert to CString"),
+            UnknownTask => write!(f, "Task failed"),
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
 pub struct Error {
-    code: ErrorCode,
+    kind: ErrorKind,
     task: ErrorTask,
     source: Option<Box<Error>>,
 }
@@ -33,126 +51,148 @@ impl Error {
         &self.task
     }
 
+    /// Get the task being attempted when the error was returned
+    pub fn kind(&self) -> &ErrorKind {
+        &self.kind
+    }
+
     /// Get the error code returned by the C API, if any
-    pub fn code(&self) -> &ErrorCode {
-        &self.code
+    pub fn code(&self) -> Option<ErrorCode> {
+        if let ErrorKind::ErrorCode(code) = self.kind {
+            Some(code)
+        } else {
+            None
+        }
     }
 
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        self.code.is_eof()
-    }
-
-    /// Construct a new error during the ToCString task
-    pub(crate) fn from_convert() -> Self {
-        Self {
-            code: ErrorCode::NoCode,
-            task: ErrorTask::ToCString(None),
-            source: None,
+        use ErrorKind::*;
+        match self.kind {
+            ErrorCode(code) => code.is_eof(),
+            _ => false,
         }
     }
 
-    /// Construct a new error during the OpenFile task
-    pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
+    pub fn with_task(self, task: ErrorTask) -> Self {
         Self {
-            code: ErrorCode::NoCode,
-            task: ErrorTask::OpenFile(path.as_ref().into(), mode),
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the ReadNumAtoms task from a C error code
-    pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::ReadNumAtoms,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Read task from a C error code
-    pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Read,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Write task from a C error code
-    pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Write,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Flush task from a C error code
-    pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Flush,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the ReadNumAtoms task from a C error code
-    pub(crate) fn with_task(self, task: ErrorTask) -> Self {
-        Self {
-            code: self.code,
+            kind: self.kind.clone(),
             task,
             source: Some(Box::new(self)),
         }
     }
+
+    /// Convert an error code and output value from a C call to a Result
+    ///
+    /// `code` should be an integer return code returned from the C API. `value` should be the
+    /// function's output, which is generally either `()` or one of its arguments. If `code`
+    /// indicates the function returned successfully, the value is returned; otherwise, the
+    /// code is converted into the appropriate `Error`.
+    pub fn check_code<T>(code: impl Into<ErrorCode>, value: T, task: ErrorTask) -> Result<T, Self> {
+        let code: ErrorCode = code.into();
+        if let ErrorCode::ExdrOk = code {
+            Ok(value)
+        } else {
+            Err(Self {
+                kind: ErrorKind::from(code),
+                task,
+                source: None,
+            })
+        }
+    }
 }
 
-impl From<std::ffi::NulError> for Error {
-    fn from(err: std::ffi::NulError) -> Self {
+impl<K: Into<ErrorKind>> From<K> for Error {
+    fn from(kind: K) -> Self {
         Self {
-            code: ErrorCode::NoCode,
-            task: ErrorTask::ToCString(Some(err)),
+            task: ErrorTask::UnknownTask,
+            kind: kind.into(),
             source: None,
         }
     }
 }
 
+impl From<std::ffi::NulError> for ErrorKind {
+    fn from(err: std::ffi::NulError) -> Self {
+        Self::NullInStr(err)
+    }
+}
+
+impl From<(&Path, FileMode)> for ErrorKind {
+    fn from(value: (&Path, FileMode)) -> Self {
+        let (path, mode) = value;
+        ErrorKind::CouldNotOpen {
+            path: path.to_owned(),
+            mode,
+        }
+    }
+}
+
+impl From<(&Frame, usize)> for ErrorKind {
+    fn from(value: (&Frame, usize)) -> Self {
+        let (frame, num_atoms) = value;
+        ErrorKind::WrongSizeFrame {
+            expected: num_atoms,
+            found: frame.coords.len(),
+        }
+    }
+}
+
+impl From<ErrorCode> for ErrorKind {
+    fn from(code: ErrorCode) -> Self {
+        ErrorKind::ErrorCode(code)
+    }
+}
+
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use ErrorTask::*;
-        match &self.task {
-            OpenFile(path, mode) => write!(
-                f,
-                "Failed to open file at {path:?} with mode {mode:?}",
-                path = path,
-                mode = mode
-            ),
-            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
-            Read => write!(f, "Failed to read trajectory"),
-            Write => write!(f, "Failed to write trajectory"),
-            Flush => write!(f, "Failed to flush trajectory"),
-            ToCString(None) => write!(
-                f,
-                "Path cannot be converted to a C string because it was invalid Unicode"
-            ),
-            ToCString(Some(_)) => write!(
-                f,
-                "Path cannot be converted to a C string because it has a null byte"
-            ),
-        }
+        write!(f, "{task}: {kind}", task = self.task, kind = self.kind)
     }
 }
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        if let Some(source) = &self.source {
-            Some(source.as_ref())
-        } else if self.code != ErrorCode::NoCode {
-            Some(&self.code)
-        } else if let ErrorTask::ToCString(Some(e)) = &self.task {
-            Some(e)
+        if let Some(err) = &self.source {
+            Some(err)
         } else {
-            None
+            use ErrorKind::*;
+            match &self.kind {
+                NullInStr(err) => Some(err),
+                _ => None,
+            }
+        }
+    }
+}
+
+#[derive(Debug, Clone, PartialEq)]
+pub enum ErrorKind {
+    /// An error code from the C API
+    ErrorCode(ErrorCode),
+    /// Passed in a frame of the wrong size
+    WrongSizeFrame { expected: usize, found: usize },
+    /// C API failed to open a file (No return code provided)
+    CouldNotOpen { path: PathBuf, mode: FileMode },
+    /// &str could not be converted to &OsStr, probably because it is invalid unicode
+    InvalidOsStr,
+    /// &OsStr could not be converted to &CStr because it had a null byte
+    NullInStr(std::ffi::NulError),
+}
+
+impl std::fmt::Display for ErrorKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorKind::*;
+        match &self {
+            ErrorCode(code) => code.fmt(f),
+            WrongSizeFrame { expected, found } => write!(
+                f,
+                "Expected frame of size {:?}, found {:?}",
+                expected, found
+            ),
+            CouldNotOpen { path, mode } => {
+                write!(f, "Could not open file at {:?} in mode {:?}", path, mode)
+            }
+            InvalidOsStr => write!(f, "CStr must be valid unicode on this platform"),
+            NullInStr(_err) => write!(f, "CStr cannot include null bytes"),
         }
     }
 }
@@ -190,8 +230,6 @@ pub enum ErrorCode {
     ExdrNr,
     /// Something unexpected happened
     UnmatchedCode(c_abi::xdrfile::BindgenTy1),
-    /// C returned no code at all
-    NoCode,
 }
 
 impl ErrorCode {
@@ -202,21 +240,6 @@ impl ErrorCode {
             _ => false,
         }
     }
-
-    /// Convert an error code and output value from a C call to a Result
-    ///
-    /// `code` should be an integer return code returned from the C API. `value` should be the
-    /// function's output, which is generally either `()` or one of its arguments. If `code`
-    /// indicates the function returned successfully, the value is returned; otherwise, the
-    /// code is converted into the appropriate `ErrorCode`.
-    pub fn check<T>(code: impl Into<Self>, value: T) -> std::result::Result<T, Self> {
-        let code = code.into();
-        if let Self::ExdrOk = code {
-            Ok(value)
-        } else {
-            Err(code)
-        }
-    }
 }
 
 impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
@@ -251,8 +274,6 @@ impl std::fmt::Display for ErrorCode {
     }
 }
 
-impl std::error::Error for ErrorCode {}
-
 /// `Result` type for errors in the `xdrfile` crate
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
@@ -262,44 +283,48 @@ mod tests {
 
     #[test]
     fn test_is_eof() {
+        use ErrorKind::ErrorCode as Code;
         let error = Error {
-            code: c_abi::xdrfile::exdrENDOFFILE.into(),
+            kind: Code(c_abi::xdrfile::exdrENDOFFILE.into()),
             task: ErrorTask::Read,
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: ErrorCode::ExdrEndOfFile,
-            task: ErrorTask::Read,
+            kind: Code(ErrorCode::ExdrEndOfFile),
+            task: ErrorTask::ReadNumAtoms,
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+            kind: Code((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
             task: ErrorTask::Read,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: 0.into(),
+            kind: Code(0.into()),
             task: ErrorTask::Write,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: 255.into(),
+            kind: Code(255.into()),
             task: ErrorTask::Flush,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: ErrorCode::NoCode,
-            task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read),
+            kind: ErrorKind::CouldNotOpen {
+                path: PathBuf::from("not/a/file"),
+                mode: FileMode::Read,
+            },
+            task: ErrorTask::OpenFile,
             source: None,
         };
         assert!(!error.is_eof());
diff --git a/src/frame.rs b/src/frame.rs
index d8e2650..805f8e7 100644
--- a/src/frame.rs
+++ b/src/frame.rs
@@ -1,7 +1,7 @@
 use std::fmt;
 
 /// A frame represents a single step in a trajectory.
-#[derive(Clone, PartialEq)]
+#[derive(Clone)]
 pub struct Frame {
     /// Number of atoms in the frame
     pub num_atoms: u32,
@@ -13,10 +13,10 @@ pub struct Frame {
     pub time: f32,
 
     /// 3x3 box vector
-    pub box_vector: [[f32; 3]; 3],
+    pub box_vector: [[f32; 3usize]; 3usize],
 
     /// 3D coordinates for N atoms where N is num_atoms
-    pub coords: Vec<[f32; 3]>,
+    pub coords: Vec<[f32; 3usize]>,
 }
 
 impl Default for Frame {
@@ -76,12 +76,6 @@ impl Frame {
     pub fn len(self: &Frame) -> usize {
         self.num_atoms as usize
     }
-
-    /// Resizes the Frame in-place so that num_atoms is equal to new_size.
-    pub fn resize(&mut self, new_size: u32) {
-        self.num_atoms = new_size;
-        self.coords.resize(new_size as usize, [0.0; 3]);
-    }
 }
 
 #[cfg(test)]
diff --git a/src/lib.rs b/src/lib.rs
index 9502d43..2db2839 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -101,8 +101,13 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    let s = path.as_ref().to_str().ok_or_else(Error::from_convert)?;
-    Ok(CString::new(s)?)
+    use ErrorKind::InvalidOsStr;
+    use ErrorTask::ToCString;
+    let s = path
+        .as_ref()
+        .to_str()
+        .ok_or_else(|| Error::from(InvalidOsStr).with_task(ToCString))?;
+    CString::new(s).map_err(|e| Error::from(e).with_task(ToCString))
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -135,7 +140,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::from_open(path, filemode))
+                Err(Error::from((path, filemode)).with_task(ErrorTask::OpenFile))
             }
         }
     }
@@ -201,25 +206,29 @@ impl XTCTrajectory {
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
-        frame.resize(
-            self.get_num_atoms()
-                .map_err(|e| e.with_task(ErrorTask::Read))?,
-        );
+
+        let num_atoms = self
+            .get_num_atoms()
+            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        if num_atoms != frame.coords.len() {
+            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+        }
+
         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,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_read)
+            Error::check_code(code, (), ErrorTask::Read)
         }
     }
 
@@ -234,14 +243,14 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_write)
+            Error::check_code(code, (), ErrorTask::Write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_flush)
+            Error::check_code(code, (), ErrorTask::Flush)
         }
     }
 
@@ -258,7 +267,7 @@ impl Trajectory for XTCTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
+                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
                 }
             })
             .clone()
@@ -300,10 +309,14 @@ impl Trajectory for TRRTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
-        frame.resize(
-            self.get_num_atoms()
-                .map_err(|e| e.with_task(ErrorTask::Read))?,
-        );
+
+        let num_atoms = self
+            .get_num_atoms()
+            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        if num_atoms != frame.coords.len() {
+            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+        }
+
         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
@@ -311,17 +324,17 @@ impl Trajectory for TRRTrajectory {
             // Similar for lambda.
             let code = xdrfile_trr::read_trr(
                 self.handle.xdrfile,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut lambda,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_read)
+            Error::check_code(code, (), ErrorTask::Read)
         }
     }
 
@@ -338,14 +351,14 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_write)
+            Error::check_code(code, (), ErrorTask::Write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_flush)
+            Error::check_code(code, (), ErrorTask::Flush)
         }
     }
 
@@ -361,7 +374,7 @@ impl Trajectory for TRRTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
+                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
                 }
             })
             .clone()
@@ -456,10 +469,9 @@ mod tests {
 
     #[test]
     pub fn test_manual_loop() -> Result<(), Box<dyn std::error::Error>> {
-        let mut frame = Frame::new();
-
         let mut xtc_frames = Vec::new();
         let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::with_capacity(xtc_traj.get_num_atoms()?);
 
         while let Ok(()) = xtc_traj.read(&mut frame) {
             xtc_frames.push(frame.clone());
@@ -486,27 +498,38 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    pub fn test_wrong_size_frame() -> Result<(), Box<dyn std::error::Error>> {
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::new();
+
+        let result = xtc_traj.read(&mut frame);
+        if let Err(e) = result {
+            assert_eq!(e.task(), &ErrorTask::Read);
+            assert!(if let ErrorKind::WrongSizeFrame { .. } = e.kind() {
+                true
+            } else {
+                false
+            });
+        } else {
+            panic!("A read with an incorrectly sized frame should not succeed")
+        }
+        Ok(())
+    }
+
     #[test]
     fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
 
-        assert_eq!(
-            result_invalid,
-            CString::new("invalid/\0path").map_err(Error::from)
-        );
-        assert!(!result_invalid.is_ok());
-        if let Err(e) = result_invalid {
-            match e.task() {
-                ErrorTask::ToCString(_) => (),
-                _ => panic!("path_to_cstring's errortask should be ErrorTask::ToCString(_)"),
-            }
+        if let Err(err) = result_invalid {
+            assert!(err.task() == &ErrorTask::ToCString);
         } else {
-            panic!("path_to_cstring on a NULL-containing string should return an error");
+            panic!("path_to_cstring should return Err if there are null bytes");
         }
 
         let result_valid = path_to_cstring("valid/path");
-
         assert_eq!(result_valid, Ok(CString::new("valid/path")?));
+
         Ok(())
     }
 
@@ -516,12 +539,18 @@ mod tests {
 
         let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            match e.task() {
-                ErrorTask::OpenFile(err_path, err_mode) => {
-                    assert_eq!(path, err_path);
-                    assert_eq!(FileMode::Read, *err_mode)
-                }
-                _ => panic!("Wrong Error type"),
+            if let (
+                ErrorTask::OpenFile,
+                ErrorKind::CouldNotOpen {
+                    path: err_path,
+                    mode: err_mode,
+                },
+            ) = (e.task(), e.kind())
+            {
+                assert_eq!(path, err_path);
+                assert_eq!(FileMode::Read, *err_mode)
+            } else {
+                panic!("Wrong Error type")
             }
         }
     }
@@ -533,7 +562,7 @@ mod tests {
         if let Err(e) = trr.get_num_atoms() {
             match e.task() {
                 ErrorTask::ReadNumAtoms => {
-                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
+                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -549,7 +578,7 @@ mod tests {
         if let Err(e) = trr.read(&mut frame) {
             match e.task() {
                 ErrorTask::Read => {
-                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
+                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }

From 4a99b4e0b040bde7616c4216fe6c22ea6f7fdff4 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 17:51:41 +1100
Subject: [PATCH 11/24] Cleaned up errors a bit

---
 src/errors.rs | 87 +++++++++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 38 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 2115bb3..e1dfd04 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -18,7 +18,7 @@ pub enum ErrorTask {
     Flush,
     /// A path was being converted to a CString
     ToCString,
-    /// Unknown task
+    /// Placeholder until a task can be provided
     UnknownTask,
 }
 
@@ -74,12 +74,23 @@ impl Error {
         }
     }
 
+    /// Change the task of the error
+    ///
+    /// Unless the current task is `UnknownTask`, the current error will be
+    /// set as the source
     pub fn with_task(self, task: ErrorTask) -> Self {
-        Self {
-            kind: self.kind.clone(),
-            task,
-            source: Some(Box::new(self)),
-        }
+        let kind;
+        let source;
+
+        if let ErrorTask::UnknownTask = self.task {
+            kind = self.kind;
+            source = None
+        } else {
+            kind = self.kind.clone();
+            source = Some(Box::new(self))
+        };
+
+        Self { kind, task, source }
     }
 
     /// Convert an error code and output value from a C call to a Result
@@ -112,38 +123,6 @@ impl<K: Into<ErrorKind>> From<K> for Error {
     }
 }
 
-impl From<std::ffi::NulError> for ErrorKind {
-    fn from(err: std::ffi::NulError) -> Self {
-        Self::NullInStr(err)
-    }
-}
-
-impl From<(&Path, FileMode)> for ErrorKind {
-    fn from(value: (&Path, FileMode)) -> Self {
-        let (path, mode) = value;
-        ErrorKind::CouldNotOpen {
-            path: path.to_owned(),
-            mode,
-        }
-    }
-}
-
-impl From<(&Frame, usize)> for ErrorKind {
-    fn from(value: (&Frame, usize)) -> Self {
-        let (frame, num_atoms) = value;
-        ErrorKind::WrongSizeFrame {
-            expected: num_atoms,
-            found: frame.coords.len(),
-        }
-    }
-}
-
-impl From<ErrorCode> for ErrorKind {
-    fn from(code: ErrorCode) -> Self {
-        ErrorKind::ErrorCode(code)
-    }
-}
-
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{task}: {kind}", task = self.task, kind = self.kind)
@@ -178,6 +157,38 @@ pub enum ErrorKind {
     NullInStr(std::ffi::NulError),
 }
 
+impl From<std::ffi::NulError> for ErrorKind {
+    fn from(err: std::ffi::NulError) -> Self {
+        Self::NullInStr(err)
+    }
+}
+
+impl From<(&Path, FileMode)> for ErrorKind {
+    fn from(value: (&Path, FileMode)) -> Self {
+        let (path, mode) = value;
+        ErrorKind::CouldNotOpen {
+            path: path.to_owned(),
+            mode,
+        }
+    }
+}
+
+impl From<(&Frame, usize)> for ErrorKind {
+    fn from(value: (&Frame, usize)) -> Self {
+        let (frame, num_atoms) = value;
+        ErrorKind::WrongSizeFrame {
+            expected: num_atoms,
+            found: frame.coords.len(),
+        }
+    }
+}
+
+impl From<ErrorCode> for ErrorKind {
+    fn from(code: ErrorCode) -> Self {
+        ErrorKind::ErrorCode(code)
+    }
+}
+
 impl std::fmt::Display for ErrorKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use ErrorKind::*;

From 420af8b369f5b214c790ca126c89322fe1a25a9d Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 18:17:23 +1100
Subject: [PATCH 12/24] Reduced role of ErrorTask to simplify errors

---
 src/errors.rs | 93 +++++++++++++++++++++++++--------------------------
 src/lib.rs    | 46 +++++++++++--------------
 2 files changed, 66 insertions(+), 73 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index e1dfd04..323a9ca 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -3,51 +3,17 @@ use crate::FileMode;
 use crate::Frame;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, Copy, PartialEq)]
-/// The task being attempted when an error was returned
-pub enum ErrorTask {
-    /// A file was being opened
-    OpenFile,
-    /// The number of atoms was being read from a file
-    ReadNumAtoms,
-    /// A frame was being read from a file
-    Read,
-    /// A frame was being written to a file
-    Write,
-    /// An file was being flushed to disk
-    Flush,
-    /// A path was being converted to a CString
-    ToCString,
-    /// Placeholder until a task can be provided
-    UnknownTask,
-}
-
-impl std::fmt::Display for ErrorTask {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use ErrorTask::*;
-        match &self {
-            OpenFile => write!(f, "Failed to open file as trajectory"),
-            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
-            Read => write!(f, "Failed to read trajectory"),
-            Write => write!(f, "Failed to write trajectory"),
-            Flush => write!(f, "Failed to flush trajectory"),
-            ToCString => write!(f, "Failed to convert to CString"),
-            UnknownTask => write!(f, "Task failed"),
-        }
-    }
-}
-
 #[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
 pub struct Error {
     kind: ErrorKind,
-    task: ErrorTask,
+    task: Option<ErrorTask>,
     source: Option<Box<Error>>,
 }
 
 impl Error {
     /// Get the task being attempted when the error was returned
-    pub fn task(&self) -> &ErrorTask {
+    pub fn task(&self) -> &Option<ErrorTask> {
         &self.task
     }
 
@@ -82,7 +48,7 @@ impl Error {
         let kind;
         let source;
 
-        if let ErrorTask::UnknownTask = self.task {
+        if let None = self.task {
             kind = self.kind;
             source = None
         } else {
@@ -90,7 +56,11 @@ impl Error {
             source = Some(Box::new(self))
         };
 
-        Self { kind, task, source }
+        Self {
+            kind,
+            task: Some(task),
+            source,
+        }
     }
 
     /// Convert an error code and output value from a C call to a Result
@@ -106,7 +76,7 @@ impl Error {
         } else {
             Err(Self {
                 kind: ErrorKind::from(code),
-                task,
+                task: Some(task),
                 source: None,
             })
         }
@@ -116,7 +86,7 @@ impl Error {
 impl<K: Into<ErrorKind>> From<K> for Error {
     fn from(kind: K) -> Self {
         Self {
-            task: ErrorTask::UnknownTask,
+            task: None,
             kind: kind.into(),
             source: None,
         }
@@ -125,7 +95,11 @@ impl<K: Into<ErrorKind>> From<K> for Error {
 
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{task}: {kind}", task = self.task, kind = self.kind)
+        if let Some(task) = self.task {
+            write!(f, "{task}: {kind}", task = task, kind = self.kind)
+        } else {
+            write!(f, "{}", self.kind)
+        }
     }
 }
 
@@ -143,6 +117,31 @@ impl std::error::Error for Error {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq)]
+/// The task being attempted if ErrorKind is ambiguous
+pub enum ErrorTask {
+    /// The number of atoms was being read from a file
+    ReadNumAtoms,
+    /// A frame was being read from a file
+    Read,
+    /// A frame was being written to a file
+    Write,
+    /// An file was being flushed to disk
+    Flush,
+}
+
+impl std::fmt::Display for ErrorTask {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorTask::*;
+        match &self {
+            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
+            Read => write!(f, "Failed to read trajectory"),
+            Write => write!(f, "Failed to write trajectory"),
+            Flush => write!(f, "Failed to flush trajectory"),
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq)]
 pub enum ErrorKind {
     /// An error code from the C API
@@ -297,35 +296,35 @@ mod tests {
         use ErrorKind::ErrorCode as Code;
         let error = Error {
             kind: Code(c_abi::xdrfile::exdrENDOFFILE.into()),
-            task: ErrorTask::Read,
+            task: Some(ErrorTask::Read),
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
             kind: Code(ErrorCode::ExdrEndOfFile),
-            task: ErrorTask::ReadNumAtoms,
+            task: Some(ErrorTask::ReadNumAtoms),
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
             kind: Code((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
-            task: ErrorTask::Read,
+            task: None,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
             kind: Code(0.into()),
-            task: ErrorTask::Write,
+            task: Some(ErrorTask::Write),
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
             kind: Code(255.into()),
-            task: ErrorTask::Flush,
+            task: Some(ErrorTask::Flush),
             source: None,
         };
         assert!(!error.is_eof());
@@ -335,7 +334,7 @@ mod tests {
                 path: PathBuf::from("not/a/file"),
                 mode: FileMode::Read,
             },
-            task: ErrorTask::OpenFile,
+            task: None,
             source: None,
         };
         assert!(!error.is_eof());
diff --git a/src/lib.rs b/src/lib.rs
index 2db2839..2a4d235 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -102,12 +102,11 @@ impl FileMode {
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
     use ErrorKind::InvalidOsStr;
-    use ErrorTask::ToCString;
     let s = path
         .as_ref()
         .to_str()
-        .ok_or_else(|| Error::from(InvalidOsStr).with_task(ToCString))?;
-    CString::new(s).map_err(|e| Error::from(e).with_task(ToCString))
+        .ok_or_else(|| Error::from(InvalidOsStr))?;
+    CString::new(s).map_err(|e| Error::from(e))
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -140,7 +139,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::from((path, filemode)).with_task(ErrorTask::OpenFile))
+                Err(Error::from((path, filemode)))
             }
         }
     }
@@ -505,7 +504,7 @@ mod tests {
 
         let result = xtc_traj.read(&mut frame);
         if let Err(e) = result {
-            assert_eq!(e.task(), &ErrorTask::Read);
+            assert_eq!(e.task(), &Some(ErrorTask::Read));
             assert!(if let ErrorKind::WrongSizeFrame { .. } = e.kind() {
                 true
             } else {
@@ -522,7 +521,11 @@ mod tests {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
 
         if let Err(err) = result_invalid {
-            assert!(err.task() == &ErrorTask::ToCString);
+            match err.kind() {
+                ErrorKind::NullInStr(_) => (),
+                ErrorKind::InvalidOsStr => (),
+                _ => panic!("Improper error type for path_to_cstring"),
+            }
         } else {
             panic!("path_to_cstring should return Err if there are null bytes");
         }
@@ -539,13 +542,10 @@ mod tests {
 
         let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            if let (
-                ErrorTask::OpenFile,
-                ErrorKind::CouldNotOpen {
-                    path: err_path,
-                    mode: err_mode,
-                },
-            ) = (e.task(), e.kind())
+            if let ErrorKind::CouldNotOpen {
+                path: err_path,
+                mode: err_mode,
+            } = e.kind()
             {
                 assert_eq!(path, err_path);
                 assert_eq!(FileMode::Read, *err_mode)
@@ -560,13 +560,10 @@ mod tests {
         let file_name = "README.md"; // not a trajectory
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.get_num_atoms() {
-            match e.task() {
-                ErrorTask::ReadNumAtoms => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
-                }
-                _ => panic!("Wrong Error type"),
-            }
-        };
+            assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
+        } else {
+            panic!("Should not be able to read number of atoms from readme");
+        }
         Ok(())
     }
 
@@ -576,12 +573,9 @@ mod tests {
         let mut frame = Frame::with_capacity(1);
         let mut trr = TRRTrajectory::open_read(file_name)?;
         if let Err(e) = trr.read(&mut frame) {
-            match e.task() {
-                ErrorTask::Read => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
-                }
-                _ => panic!("Wrong Error type"),
-            }
+            assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
+        } else {
+            panic!("Should not be able to read number of atoms from readme");
         }
         Ok(())
     }

From 51635f2583412bffa5012f2d8fa73d349325b3d3 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 18:50:14 +1100
Subject: [PATCH 13/24] Restricted ErrorTask to only being relevant to C API
 errors

---
 src/errors.rs | 159 ++++++++++++++++++++++----------------------------
 src/lib.rs    |  13 ++---
 2 files changed, 73 insertions(+), 99 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 323a9ca..e0863ef 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -7,16 +7,10 @@ use std::path::{Path, PathBuf};
 /// Error type for the xdrfile library
 pub struct Error {
     kind: ErrorKind,
-    task: Option<ErrorTask>,
     source: Option<Box<Error>>,
 }
 
 impl Error {
-    /// Get the task being attempted when the error was returned
-    pub fn task(&self) -> &Option<ErrorTask> {
-        &self.task
-    }
-
     /// Get the task being attempted when the error was returned
     pub fn kind(&self) -> &ErrorKind {
         &self.kind
@@ -24,43 +18,26 @@ impl Error {
 
     /// Get the error code returned by the C API, if any
     pub fn code(&self) -> Option<ErrorCode> {
-        if let ErrorKind::ErrorCode(code) = self.kind {
+        if let ErrorKind::CApiError { code, .. } = self.kind {
             Some(code)
         } else {
             None
         }
     }
 
+    /// Get the task being attempted when the C API returned an error, if any
+    pub fn task(&self) -> Option<ErrorTask> {
+        if let ErrorKind::CApiError { task, .. } = self.kind {
+            Some(task)
+        } else {
+            None
+        }
+    }
+
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        use ErrorKind::*;
-        match self.kind {
-            ErrorCode(code) => code.is_eof(),
-            _ => false,
-        }
-    }
-
-    /// Change the task of the error
-    ///
-    /// Unless the current task is `UnknownTask`, the current error will be
-    /// set as the source
-    pub fn with_task(self, task: ErrorTask) -> Self {
-        let kind;
-        let source;
-
-        if let None = self.task {
-            kind = self.kind;
-            source = None
-        } else {
-            kind = self.kind.clone();
-            source = Some(Box::new(self))
-        };
-
-        Self {
-            kind,
-            task: Some(task),
-            source,
-        }
+        self.code().map_or(false, |e| e.is_eof())
+            | self.source.as_ref().map_or(false, |e| e.is_eof())
     }
 
     /// Convert an error code and output value from a C call to a Result
@@ -75,8 +52,7 @@ impl Error {
             Ok(value)
         } else {
             Err(Self {
-                kind: ErrorKind::from(code),
-                task: Some(task),
+                kind: ErrorKind::CApiError { code, task },
                 source: None,
             })
         }
@@ -86,7 +62,6 @@ impl Error {
 impl<K: Into<ErrorKind>> From<K> for Error {
     fn from(kind: K) -> Self {
         Self {
-            task: None,
             kind: kind.into(),
             source: None,
         }
@@ -95,11 +70,7 @@ impl<K: Into<ErrorKind>> From<K> for Error {
 
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        if let Some(task) = self.task {
-            write!(f, "{task}: {kind}", task = task, kind = self.kind)
-        } else {
-            write!(f, "{}", self.kind)
-        }
+        self.kind.fmt(f)
     }
 }
 
@@ -117,35 +88,10 @@ impl std::error::Error for Error {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq)]
-/// The task being attempted if ErrorKind is ambiguous
-pub enum ErrorTask {
-    /// The number of atoms was being read from a file
-    ReadNumAtoms,
-    /// A frame was being read from a file
-    Read,
-    /// A frame was being written to a file
-    Write,
-    /// An file was being flushed to disk
-    Flush,
-}
-
-impl std::fmt::Display for ErrorTask {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use ErrorTask::*;
-        match &self {
-            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
-            Read => write!(f, "Failed to read trajectory"),
-            Write => write!(f, "Failed to write trajectory"),
-            Flush => write!(f, "Failed to flush trajectory"),
-        }
-    }
-}
-
 #[derive(Debug, Clone, PartialEq)]
 pub enum ErrorKind {
     /// An error code from the C API
-    ErrorCode(ErrorCode),
+    CApiError { code: ErrorCode, task: ErrorTask },
     /// Passed in a frame of the wrong size
     WrongSizeFrame { expected: usize, found: usize },
     /// C API failed to open a file (No return code provided)
@@ -182,17 +128,16 @@ impl From<(&Frame, usize)> for ErrorKind {
     }
 }
 
-impl From<ErrorCode> for ErrorKind {
-    fn from(code: ErrorCode) -> Self {
-        ErrorKind::ErrorCode(code)
-    }
-}
-
 impl std::fmt::Display for ErrorKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use ErrorKind::*;
         match &self {
-            ErrorCode(code) => code.fmt(f),
+            CApiError { code, task } => write!(
+                f,
+                "Error while {task}: C API returned error code {code}",
+                task = task,
+                code = code
+            ),
             WrongSizeFrame { expected, found } => write!(
                 f,
                 "Expected frame of size {:?}, found {:?}",
@@ -207,6 +152,31 @@ impl std::fmt::Display for ErrorKind {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq)]
+/// The task being attempted when the C API returns an error
+pub enum ErrorTask {
+    /// The number of atoms was being read from a file
+    ReadNumAtoms,
+    /// A frame was being read from a file
+    Read,
+    /// A frame was being written to a file
+    Write,
+    /// An file was being flushed to disk
+    Flush,
+}
+
+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"),
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Copy)]
 /// Error codes returned from the C API
 pub enum ErrorCode {
@@ -277,9 +247,9 @@ impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
 impl std::fmt::Display for ErrorCode {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         if let Self::UnmatchedCode(i) = self {
-            write!(f, "C API returned error code {}", i)
+            write!(f, "{}", i)
         } else {
-            write!(f, "C API returned error code {:?}", self)
+            write!(f, "{:?}", self)
         }
     }
 }
@@ -293,38 +263,48 @@ mod tests {
 
     #[test]
     fn test_is_eof() {
-        use ErrorKind::ErrorCode as Code;
+        use ErrorKind::CApiError;
         let error = Error {
-            kind: Code(c_abi::xdrfile::exdrENDOFFILE.into()),
-            task: Some(ErrorTask::Read),
+            kind: CApiError {
+                code: c_abi::xdrfile::exdrENDOFFILE.into(),
+                task: ErrorTask::Read,
+            },
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            kind: Code(ErrorCode::ExdrEndOfFile),
-            task: Some(ErrorTask::ReadNumAtoms),
+            kind: CApiError {
+                code: ErrorCode::ExdrEndOfFile,
+                task: ErrorTask::Read,
+            },
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            kind: Code((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
-            task: None,
+            kind: CApiError {
+                code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+                task: ErrorTask::Read,
+            },
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            kind: Code(0.into()),
-            task: Some(ErrorTask::Write),
+            kind: CApiError {
+                code: 0.into(),
+                task: ErrorTask::Read,
+            },
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            kind: Code(255.into()),
-            task: Some(ErrorTask::Flush),
+            kind: CApiError {
+                code: 255.into(),
+                task: ErrorTask::Read,
+            },
             source: None,
         };
         assert!(!error.is_eof());
@@ -334,7 +314,6 @@ mod tests {
                 path: PathBuf::from("not/a/file"),
                 mode: FileMode::Read,
             },
-            task: None,
             source: None,
         };
         assert!(!error.is_eof());
diff --git a/src/lib.rs b/src/lib.rs
index 2a4d235..0dd84a6 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -206,11 +206,9 @@ impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
 
-        let num_atoms = self
-            .get_num_atoms()
-            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        let num_atoms = self.get_num_atoms()? as usize;
         if num_atoms != frame.coords.len() {
-            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+            return Err(Error::from((&*frame, num_atoms)));
         }
 
         unsafe {
@@ -309,11 +307,9 @@ impl Trajectory for TRRTrajectory {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
 
-        let num_atoms = self
-            .get_num_atoms()
-            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        let num_atoms = self.get_num_atoms()? as usize;
         if num_atoms != frame.coords.len() {
-            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+            return Err(Error::from((&*frame, num_atoms)));
         }
 
         unsafe {
@@ -504,7 +500,6 @@ mod tests {
 
         let result = xtc_traj.read(&mut frame);
         if let Err(e) = result {
-            assert_eq!(e.task(), &Some(ErrorTask::Read));
             assert!(if let ErrorKind::WrongSizeFrame { .. } = e.kind() {
                 true
             } else {

From ce16b20b677c732094b9533aad6dde9aa1f8f34b Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 18:52:16 +1100
Subject: [PATCH 14/24] Slightly clearer docs

---
 src/errors.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index e0863ef..678fcac 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -96,9 +96,9 @@ pub enum ErrorKind {
     WrongSizeFrame { expected: usize, found: usize },
     /// C API failed to open a file (No return code provided)
     CouldNotOpen { path: PathBuf, mode: FileMode },
-    /// &str could not be converted to &OsStr, probably because it is invalid unicode
+    /// A path could not be converted to &OsStr, probably because it is invalid unicode
     InvalidOsStr,
-    /// &OsStr could not be converted to &CStr because it had a null byte
+    /// A path could not be converted to &CStr because it had a null byte
     NullInStr(std::ffi::NulError),
 }
 
@@ -146,8 +146,8 @@ impl std::fmt::Display for ErrorKind {
             CouldNotOpen { path, mode } => {
                 write!(f, "Could not open file at {:?} in mode {:?}", path, mode)
             }
-            InvalidOsStr => write!(f, "CStr must be valid unicode on this platform"),
-            NullInStr(_err) => write!(f, "CStr cannot include null bytes"),
+            InvalidOsStr => write!(f, "Paths must be valid unicode on this platform"),
+            NullInStr(_err) => write!(f, "Paths cannot include null bytes"),
         }
     }
 }

From 85b106f5d875b9fd7e906ebdbce83da7de016bcb Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 19:34:58 +1100
Subject: [PATCH 15/24] Returned Error to being an enum, as other fields were
 unused

---
 src/errors.rs | 146 ++++++++++++++++----------------------------------
 src/lib.rs    |  20 +++----
 2 files changed, 55 insertions(+), 111 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 678fcac..55165f8 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -3,23 +3,26 @@ use crate::FileMode;
 use crate::Frame;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
-pub struct Error {
-    kind: ErrorKind,
-    source: Option<Box<Error>>,
+#[derive(Debug, Clone, PartialEq)]
+pub enum Error {
+    /// An error code from the C API
+    CApiError { code: ErrorCode, task: ErrorTask },
+    /// Passed in a frame of the wrong size
+    WrongSizeFrame { expected: usize, found: usize },
+    /// C API failed to open a file (No return code provided)
+    CouldNotOpen { path: PathBuf, mode: FileMode },
+    /// A path could not be converted to &OsStr, probably because it is invalid unicode
+    InvalidOsStr,
+    /// A path could not be converted to &CStr because it had a null byte
+    NullInStr(std::ffi::NulError),
 }
 
 impl Error {
-    /// Get the task being attempted when the error was returned
-    pub fn kind(&self) -> &ErrorKind {
-        &self.kind
-    }
-
     /// Get the error code returned by the C API, if any
     pub fn code(&self) -> Option<ErrorCode> {
-        if let ErrorKind::CApiError { code, .. } = self.kind {
-            Some(code)
+        if let Error::CApiError { code, .. } = self {
+            Some(*code)
         } else {
             None
         }
@@ -27,8 +30,8 @@ impl Error {
 
     /// Get the task being attempted when the C API returned an error, if any
     pub fn task(&self) -> Option<ErrorTask> {
-        if let ErrorKind::CApiError { task, .. } = self.kind {
-            Some(task)
+        if let Error::CApiError { task, .. } = self {
+            Some(*task)
         } else {
             None
         }
@@ -37,7 +40,6 @@ impl Error {
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
         self.code().map_or(false, |e| e.is_eof())
-            | self.source.as_ref().map_or(false, |e| e.is_eof())
     }
 
     /// Convert an error code and output value from a C call to a Result
@@ -51,86 +53,50 @@ impl Error {
         if let ErrorCode::ExdrOk = code {
             Ok(value)
         } else {
-            Err(Self {
-                kind: ErrorKind::CApiError { code, task },
-                source: None,
-            })
+            Err(Self::CApiError { code, task })
         }
     }
 }
 
-impl<K: Into<ErrorKind>> From<K> for Error {
-    fn from(kind: K) -> Self {
-        Self {
-            kind: kind.into(),
-            source: None,
-        }
-    }
-}
-
-impl std::fmt::Display for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.kind.fmt(f)
-    }
-}
-
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        if let Some(err) = &self.source {
-            Some(err)
-        } else {
-            use ErrorKind::*;
-            match &self.kind {
-                NullInStr(err) => Some(err),
-                _ => None,
-            }
+        use Error::*;
+        match &self {
+            NullInStr(err) => Some(err),
+            _ => None,
         }
     }
 }
 
-#[derive(Debug, Clone, PartialEq)]
-pub enum ErrorKind {
-    /// An error code from the C API
-    CApiError { code: ErrorCode, task: ErrorTask },
-    /// Passed in a frame of the wrong size
-    WrongSizeFrame { expected: usize, found: usize },
-    /// C API failed to open a file (No return code provided)
-    CouldNotOpen { path: PathBuf, mode: FileMode },
-    /// A path could not be converted to &OsStr, probably because it is invalid unicode
-    InvalidOsStr,
-    /// A path could not be converted to &CStr because it had a null byte
-    NullInStr(std::ffi::NulError),
-}
-
-impl From<std::ffi::NulError> for ErrorKind {
+impl From<std::ffi::NulError> for Error {
     fn from(err: std::ffi::NulError) -> Self {
         Self::NullInStr(err)
     }
 }
 
-impl From<(&Path, FileMode)> for ErrorKind {
+impl From<(&Path, FileMode)> for Error {
     fn from(value: (&Path, FileMode)) -> Self {
         let (path, mode) = value;
-        ErrorKind::CouldNotOpen {
+        Error::CouldNotOpen {
             path: path.to_owned(),
             mode,
         }
     }
 }
 
-impl From<(&Frame, usize)> for ErrorKind {
+impl From<(&Frame, usize)> for Error {
     fn from(value: (&Frame, usize)) -> Self {
         let (frame, num_atoms) = value;
-        ErrorKind::WrongSizeFrame {
+        Error::WrongSizeFrame {
             expected: num_atoms,
             found: frame.coords.len(),
         }
     }
 }
 
-impl std::fmt::Display for ErrorKind {
+impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use ErrorKind::*;
+        use Error::*;
         match &self {
             CApiError { code, task } => write!(
                 f,
@@ -263,58 +229,40 @@ mod tests {
 
     #[test]
     fn test_is_eof() {
-        use ErrorKind::CApiError;
-        let error = Error {
-            kind: CApiError {
-                code: c_abi::xdrfile::exdrENDOFFILE.into(),
-                task: ErrorTask::Read,
-            },
-            source: None,
+        use Error::CApiError;
+        let error = CApiError {
+            code: c_abi::xdrfile::exdrENDOFFILE.into(),
+            task: ErrorTask::Read,
         };
         assert!(error.is_eof());
 
-        let error = Error {
-            kind: CApiError {
-                code: ErrorCode::ExdrEndOfFile,
-                task: ErrorTask::Read,
-            },
-            source: None,
+        let error = CApiError {
+            code: ErrorCode::ExdrEndOfFile,
+            task: ErrorTask::Read,
         };
         assert!(error.is_eof());
 
-        let error = Error {
-            kind: CApiError {
-                code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
-                task: ErrorTask::Read,
-            },
-            source: None,
+        let error = CApiError {
+            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+            task: ErrorTask::Read,
         };
         assert!(!error.is_eof());
 
-        let error = Error {
-            kind: CApiError {
-                code: 0.into(),
-                task: ErrorTask::Read,
-            },
-            source: None,
+        let error = CApiError {
+            code: 0.into(),
+            task: ErrorTask::Read,
         };
         assert!(!error.is_eof());
 
-        let error = Error {
-            kind: CApiError {
-                code: 255.into(),
-                task: ErrorTask::Read,
-            },
-            source: None,
+        let error = CApiError {
+            code: 255.into(),
+            task: ErrorTask::Read,
         };
         assert!(!error.is_eof());
 
-        let error = Error {
-            kind: ErrorKind::CouldNotOpen {
-                path: PathBuf::from("not/a/file"),
-                mode: FileMode::Read,
-            },
-            source: None,
+        let error = Error::CouldNotOpen {
+            path: PathBuf::from("not/a/file"),
+            mode: FileMode::Read,
         };
         assert!(!error.is_eof());
     }
diff --git a/src/lib.rs b/src/lib.rs
index 0dd84a6..8e8eff8 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -101,11 +101,7 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    use ErrorKind::InvalidOsStr;
-    let s = path
-        .as_ref()
-        .to_str()
-        .ok_or_else(|| Error::from(InvalidOsStr))?;
+    let s = path.as_ref().to_str().ok_or_else(|| Error::InvalidOsStr)?;
     CString::new(s).map_err(|e| Error::from(e))
 }
 
@@ -500,7 +496,7 @@ mod tests {
 
         let result = xtc_traj.read(&mut frame);
         if let Err(e) = result {
-            assert!(if let ErrorKind::WrongSizeFrame { .. } = e.kind() {
+            assert!(if let Error::WrongSizeFrame { .. } = e {
                 true
             } else {
                 false
@@ -516,9 +512,9 @@ mod tests {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
 
         if let Err(err) = result_invalid {
-            match err.kind() {
-                ErrorKind::NullInStr(_) => (),
-                ErrorKind::InvalidOsStr => (),
+            match err {
+                Error::NullInStr(_) => (),
+                Error::InvalidOsStr => (),
                 _ => panic!("Improper error type for path_to_cstring"),
             }
         } else {
@@ -537,13 +533,13 @@ mod tests {
 
         let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            if let ErrorKind::CouldNotOpen {
+            if let Error::CouldNotOpen {
                 path: err_path,
                 mode: err_mode,
-            } = e.kind()
+            } = e
             {
                 assert_eq!(path, err_path);
-                assert_eq!(FileMode::Read, *err_mode)
+                assert_eq!(FileMode::Read, err_mode)
             } else {
                 panic!("Wrong Error type")
             }

From 323ba0b88c2ee88de618b0b7bfca99fc2925bb3e Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 19:49:58 +1100
Subject: [PATCH 16/24] Clarified error message when read_num_atoms() fails
 during read()

---
 src/errors.rs | 24 +++++++++++++++++++++---
 src/lib.rs    |  8 ++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 55165f8..c03a07a 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -7,22 +7,35 @@ use std::path::{Path, PathBuf};
 #[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, probably because it is invalid unicode
     InvalidOsStr,
     /// A path could not be converted to &CStr because it had a null byte
     NullInStr(std::ffi::NulError),
+    CouldNotCheckNAtoms(Box<Error>),
 }
 
 impl Error {
     /// Get the error code returned by the C API, if any
     pub fn code(&self) -> Option<ErrorCode> {
+        use std::error::Error as _;
         if let Error::CApiError { code, .. } = self {
             Some(*code)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::code)
         } else {
             None
         }
@@ -63,6 +76,7 @@ impl std::error::Error for Error {
         use Error::*;
         match &self {
             NullInStr(err) => Some(err),
+            CouldNotCheckNAtoms(err) => Some(err.as_ref()),
             _ => None,
         }
     }
@@ -114,6 +128,10 @@ 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!(
+                f,
+                "Failed to check number of atoms in trajectory while reading a frame"
+            ),
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 8e8eff8..66b9f99 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -202,7 +202,9 @@ impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
 
-        let num_atoms = self.get_num_atoms()? as usize;
+        let num_atoms = self
+            .get_num_atoms()
+            .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             return Err(Error::from((&*frame, num_atoms)));
         }
@@ -303,7 +305,9 @@ impl Trajectory for TRRTrajectory {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
 
-        let num_atoms = self.get_num_atoms()? as usize;
+        let num_atoms = self
+            .get_num_atoms()
+            .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             return Err(Error::from((&*frame, num_atoms)));
         }

From 7ee04620f6dfce69ed888d362851d093a64c28ec Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Tue, 10 Nov 2020 20:11:15 +1100
Subject: [PATCH 17/24] Error::task() now checks sources

---
 src/errors.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/errors.rs b/src/errors.rs
index c03a07a..cf5d1cd 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -43,8 +43,11 @@ impl Error {
 
     /// Get the task being attempted when the C API returned an error, if any
     pub fn task(&self) -> Option<ErrorTask> {
+        use std::error::Error as _;
         if let Error::CApiError { task, .. } = self {
             Some(*task)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::task)
         } else {
             None
         }

From 56a15b7a4161fa476b9fdafcb73ef3a4a05f1c6e Mon Sep 17 00:00:00 2001
From: daniel <bauer@cbs.tu-darmstadt.de>
Date: Tue, 10 Nov 2020 16:21:29 +0100
Subject: [PATCH 18/24] code cleanup

---
 src/errors.rs | 21 +++++++++------------
 src/lib.rs    | 15 +++++++--------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index cf5d1cd..99cb1ba 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::path::{Path, PathBuf};
+use std::error::Error as StdError;
 
 /// Error type for the xdrfile library
 #[derive(Debug, Clone, PartialEq)]
@@ -21,7 +22,7 @@ pub enum Error {
         path: PathBuf,
         mode: FileMode,
     },
-    /// A path could not be converted to &OsStr, probably because it is invalid unicode
+    /// 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),
@@ -31,7 +32,7 @@ pub enum Error {
 impl Error {
     /// Get the error code returned by the C API, if any
     pub fn code(&self) -> Option<ErrorCode> {
-        use std::error::Error as _;
+        // use std::error::Error as _;
         if let Error::CApiError { code, .. } = self {
             Some(*code)
         } else if let Some(e) = self.source() {
@@ -43,7 +44,7 @@ impl Error {
 
     /// Get the task being attempted when the C API returned an error, if any
     pub fn task(&self) -> Option<ErrorTask> {
-        use std::error::Error as _;
+        // use std::error::Error as _;
         if let Error::CApiError { task, .. } = self {
             Some(*task)
         } else if let Some(e) = self.source() {
@@ -76,10 +77,9 @@ impl Error {
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        use Error::*;
         match &self {
-            NullInStr(err) => Some(err),
-            CouldNotCheckNAtoms(err) => Some(err.as_ref()),
+            Error::NullInStr(err) => Some(err),
+            Error::CouldNotCheckNAtoms(err) => Some(err.as_ref()),
             _ => None,
         }
     }
@@ -139,8 +139,8 @@ impl std::fmt::Display for Error {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq)]
 /// The task being attempted when the C API returns an error
+#[derive(Debug, Clone, Copy, PartialEq)]
 pub enum ErrorTask {
     /// The number of atoms was being read from a file
     ReadNumAtoms,
@@ -164,8 +164,8 @@ impl std::fmt::Display for ErrorTask {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Copy)]
 /// Error codes returned from the C API
+#[derive(Debug, Clone, PartialEq, Copy)]
 pub enum ErrorCode {
     /// No error, C API returned successfully
     ExdrOk,
@@ -202,10 +202,7 @@ pub enum ErrorCode {
 impl ErrorCode {
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        match self {
-            Self::ExdrEndOfFile => true,
-            _ => false,
-        }
+        matches!(self, Self::ExdrEndOfFile)
     }
 }
 
diff --git a/src/lib.rs b/src/lib.rs
index 66b9f99..2cc3a96 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -101,8 +101,8 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    let s = path.as_ref().to_str().ok_or_else(|| Error::InvalidOsStr)?;
-    CString::new(s).map_err(|e| Error::from(e))
+    let s = path.as_ref().to_str().ok_or(Error::InvalidOsStr)?;
+    CString::new(s).map_err(Error::from)
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -135,7 +135,10 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::from((path, filemode)))
+                Err(Error::CouldNotOpen {
+                    path: path.to_owned(),
+                    mode: filemode
+                })
             }
         }
     }
@@ -500,11 +503,7 @@ mod tests {
 
         let result = xtc_traj.read(&mut frame);
         if let Err(e) = result {
-            assert!(if let Error::WrongSizeFrame { .. } = e {
-                true
-            } else {
-                false
-            });
+            assert!(matches!(e, Error::WrongSizeFrame { .. }));
         } else {
             panic!("A read with an incorrectly sized frame should not succeed")
         }

From 7a22031bee4292d7eb8ca42c7fcb85f02f2cea07 Mon Sep 17 00:00:00 2001
From: daniel <bauer@cbs.tu-darmstadt.de>
Date: Tue, 10 Nov 2020 17:22:43 +0100
Subject: [PATCH 19/24] move check_code to lib

---
 src/errors.rs | 22 ++++++-----------
 src/lib.rs    | 68 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 27 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 99cb1ba..f92011f 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -58,21 +58,6 @@ impl Error {
     pub fn is_eof(&self) -> bool {
         self.code().map_or(false, |e| e.is_eof())
     }
-
-    /// Convert an error code and output value from a C call to a Result
-    ///
-    /// `code` should be an integer return code returned from the C API. `value` should be the
-    /// function's output, which is generally either `()` or one of its arguments. If `code`
-    /// indicates the function returned successfully, the value is returned; otherwise, the
-    /// code is converted into the appropriate `Error`.
-    pub fn check_code<T>(code: impl Into<ErrorCode>, value: T, task: ErrorTask) -> Result<T, Self> {
-        let code: ErrorCode = code.into();
-        if let ErrorCode::ExdrOk = code {
-            Ok(value)
-        } else {
-            Err(Self::CApiError { code, task })
-        }
-    }
 }
 
 impl std::error::Error for Error {
@@ -85,6 +70,13 @@ impl std::error::Error for Error {
     }
 }
 
+impl From<(ErrorCode, ErrorTask)> for Error {
+    fn from(value: (ErrorCode, ErrorTask)) -> Self {
+        let (code, task) = value;
+        Self::CApiError { code, task }
+    }
+}
+
 impl From<std::ffi::NulError> for Error {
     fn from(err: std::ffi::NulError) -> Self {
         Self::NullInStr(err)
diff --git a/src/lib.rs b/src/lib.rs
index 2cc3a96..3dab368 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -105,6 +105,21 @@ fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
     CString::new(s).map_err(Error::from)
 }
 
+/// 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;
+/// otherwise, the code is converted into the appropriate `Error`.
+pub fn check_code(code: impl Into<ErrorCode>, task: ErrorTask) -> Option<Error> {
+    let code: ErrorCode = code.into();
+    if let ErrorCode::ExdrOk = code {
+        None
+    } else {
+        Some(Error::from((code, task)))
+    }
+}
+
+
 /// A safe wrapper around the c implementation of an XDRFile
 struct XDRFile {
     xdrfile: *mut XDRFILE,
@@ -135,10 +150,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::CouldNotOpen {
-                    path: path.to_owned(),
-                    mode: filemode
-                })
+                Err(Error::from((path, filemode)))
             }
         }
     }
@@ -226,7 +238,11 @@ impl Trajectory for XTCTrajectory {
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            Error::check_code(code, (), ErrorTask::Read)
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
@@ -241,14 +257,22 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            Error::check_code(code, (), ErrorTask::Write)
+            if let Some(err) = check_code(code, ErrorTask::Write) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            Error::check_code(code, (), ErrorTask::Flush)
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
@@ -265,7 +289,11 @@ impl Trajectory for XTCTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
+                    if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
+                        Err(err)
+                    } else {
+                        Ok(num_atoms as u32)
+                    }
                 }
             })
             .clone()
@@ -332,7 +360,11 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            Error::check_code(code, (), ErrorTask::Read)
+            if let Some(err) = check_code(code, ErrorTask::Read) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
@@ -349,14 +381,22 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            Error::check_code(code, (), ErrorTask::Write)
+            if let Some(err) = check_code(code, ErrorTask::Write) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            Error::check_code(code, (), ErrorTask::Flush)
+            if let Some(err) = check_code(code, ErrorTask::Flush) {
+                Err(err)
+            } else {
+                Ok(())
+            }
         }
     }
 
@@ -372,7 +412,11 @@ impl Trajectory for TRRTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
+                    if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
+                        Err(err)
+                    } else {
+                        Ok(num_atoms as u32)
+                    }
                 }
             })
             .clone()

From 0d887fc515c68424ffe63c720acf0b6a0049da9e Mon Sep 17 00:00:00 2001
From: daniel <bauer@cbs.tu-darmstadt.de>
Date: Tue, 10 Nov 2020 17:46:11 +0100
Subject: [PATCH 20/24] tests

---
 src/errors.rs | 30 ++++++++++++++++++++++++++++--
 src/lib.rs    | 11 +++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index f92011f..b875cce 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -32,7 +32,6 @@ pub enum Error {
 impl Error {
     /// Get the error code returned by the C API, if any
     pub fn code(&self) -> Option<ErrorCode> {
-        // use std::error::Error as _;
         if let Error::CApiError { code, .. } = self {
             Some(*code)
         } else if let Some(e) = self.source() {
@@ -44,7 +43,6 @@ impl Error {
 
     /// Get the task being attempted when the C API returned an error, if any
     pub fn task(&self) -> Option<ErrorTask> {
-        // use std::error::Error as _;
         if let Error::CApiError { task, .. } = self {
             Some(*task)
         } else if let Some(e) = self.source() {
@@ -236,6 +234,8 @@ pub type Result<T, E = Error> = std::result::Result<T, E>;
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::ffi::NulError;
+    use std::ffi::CString;
 
     #[test]
     fn test_is_eof() {
@@ -276,4 +276,30 @@ mod tests {
         };
         assert!(!error.is_eof());
     }
+
+    #[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 };
+        let err = Error::from((code, task));
+        assert_eq!(expected, err);
+
+        let path = Path::new(".");
+        let mode = FileMode::Read;
+        let expected = Error::CouldNotOpen{path: path.to_path_buf(), mode: mode.to_owned()};
+        let err = Error::from((path, mode));
+        assert_eq!(expected, err);
+
+        let frame = Frame::with_capacity(0);
+        let expected = Error::WrongSizeFrame{expected: 10, found: 0};
+        let err = Error::from((&frame, 10));
+        assert_eq!(expected, err);
+    }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 3dab368..46e5fad 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -662,4 +662,15 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_check_code() {
+        let code: ErrorCode = 0.into();
+        assert!(!check_code(code, ErrorTask::Read).is_some());
+
+        for i in vec![1, 10, 100, 1000] {
+            let code: ErrorCode = i.into();
+            assert!(check_code(code, ErrorTask::Read).is_some());
+        }
+    }
 }

From 6d911fae0b3eacc6c7b6ba2777de19c9eabe8ead Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Wed, 11 Nov 2020 16:40:37 +1100
Subject: [PATCH 21/24] Clearer errors if get_num_atoms fails during into_iter

---
 src/iterator.rs | 53 +++++++++++++++++++++++++++++--------------------
 src/lib.rs      |  9 ++++-----
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/src/iterator.rs b/src/iterator.rs
index 5f69fab..1bea8cc 100644
--- a/src/iterator.rs
+++ b/src/iterator.rs
@@ -1,20 +1,26 @@
 use crate::*;
 use std::rc::Rc;
 
+fn into_iter_inner<T: Trajectory>(mut traj: T) -> TrajectoryIterator<T> {
+    let num_atoms = traj.get_num_atoms();
+    let frame = match &num_atoms {
+        Ok(num_atoms) => Frame::with_capacity(*num_atoms),
+        Err(_) => Frame::new(),
+    };
+    TrajectoryIterator {
+        trajectory: traj,
+        item: Rc::new(frame),
+        has_error: false,
+        num_atoms,
+    }
+}
+
 impl IntoIterator for XTCTrajectory {
     type Item = Result<Rc<Frame>>;
     type IntoIter = TrajectoryIterator<XTCTrajectory>;
 
-    fn into_iter(mut self) -> Self::IntoIter {
-        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),
-            has_error: false,
-        }
+    fn into_iter(self) -> Self::IntoIter {
+        into_iter_inner(self)
     }
 }
 
@@ -22,16 +28,8 @@ impl IntoIterator for TRRTrajectory {
     type Item = Result<Rc<Frame>>;
     type IntoIter = TrajectoryIterator<TRRTrajectory>;
 
-    fn into_iter(mut self) -> Self::IntoIter {
-        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),
-            has_error: false,
-        }
+    fn into_iter(self) -> Self::IntoIter {
+        into_iter_inner(self)
     }
 }
 
@@ -44,6 +42,7 @@ pub struct TrajectoryIterator<T> {
     trajectory: T,
     item: Rc<Frame>,
     has_error: bool,
+    num_atoms: Result<u32>,
 }
 
 impl<T> Iterator for TrajectoryIterator<T>
@@ -53,19 +52,29 @@ where
     type Item = Result<Rc<Frame>>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        // Reuse old frame
         if self.has_error {
             return None;
         }
 
+        // If we couldn't read the number of frames when we called into_iter, return that error now
+        let num_atoms = match &self.num_atoms {
+            &Ok(n) => n,
+            Err(e) => {
+                self.has_error = true;
+                return Some(Err(e.clone()));
+            }
+        };
+
+        // Reuse old frame
         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));
+                self.item = Rc::new(Frame::with_capacity(num_atoms));
                 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(e) if e.is_eof() => None,
diff --git a/src/lib.rs b/src/lib.rs
index 46e5fad..cbd1fe4 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -102,7 +102,7 @@ impl FileMode {
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
     let s = path.as_ref().to_str().ok_or(Error::InvalidOsStr)?;
-    CString::new(s).map_err(Error::from)
+    Ok(CString::new(s)?)
 }
 
 /// Convert an error code from a C call to an Error
@@ -119,7 +119,6 @@ pub fn check_code(code: impl Into<ErrorCode>, task: ErrorTask) -> Option<Error>
     }
 }
 
-
 /// A safe wrapper around the c implementation of an XDRFile
 struct XDRFile {
     xdrfile: *mut XDRFILE,
@@ -150,7 +149,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::from((path, filemode)))
+                Err((path, filemode))?
             }
         }
     }
@@ -221,7 +220,7 @@ impl Trajectory for XTCTrajectory {
             .get_num_atoms()
             .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
-            return Err(Error::from((&*frame, num_atoms)));
+            Err((&*frame, num_atoms))?;
         }
 
         unsafe {
@@ -340,7 +339,7 @@ impl Trajectory for TRRTrajectory {
             .get_num_atoms()
             .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
-            return Err(Error::from((&*frame, num_atoms)));
+            Err((&*frame, num_atoms))?;
         }
 
         unsafe {

From 53dd997129f762048e45dd5fba3a46a7c2829a62 Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Wed, 11 Nov 2020 17:11:55 +1100
Subject: [PATCH 22/24] Broke out next_inner

---
 src/errors.rs   | 28 ++++++++++++++++++++--------
 src/iterator.rs | 39 ++++++++++++++++++++++-----------------
 src/lib.rs      | 12 ++++++------
 3 files changed, 48 insertions(+), 31 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index b875cce..1e148cd 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -1,8 +1,8 @@
 use crate::c_abi;
 use crate::FileMode;
 use crate::Frame;
-use std::path::{Path, PathBuf};
 use std::error::Error as StdError;
+use std::path::{Path, PathBuf};
 
 /// Error type for the xdrfile library
 #[derive(Debug, Clone, PartialEq)]
@@ -26,7 +26,8 @@ pub enum Error {
     InvalidOsStr,
     /// A path could not be converted to &CStr because it had a null byte
     NullInStr(std::ffi::NulError),
-    CouldNotCheckNAtoms(Box<Error>),
+    CheckNAtomsDuringRead(Box<Error>),
+    CheckNAtomsDuringIter(Box<Error>),
 }
 
 impl Error {
@@ -60,9 +61,10 @@ impl Error {
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use Error::*;
         match &self {
-            Error::NullInStr(err) => Some(err),
-            Error::CouldNotCheckNAtoms(err) => Some(err.as_ref()),
+            NullInStr(err) => Some(err),
+            CheckNAtomsDuringRead(err) | CheckNAtomsDuringIter(err) => Some(err.as_ref()),
             _ => None,
         }
     }
@@ -121,10 +123,14 @@ 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!(
+            CheckNAtomsDuringRead(_err) => write!(
                 f,
                 "Failed to check number of atoms in trajectory while reading a frame"
             ),
+            CheckNAtomsDuringIter(_err) => write!(
+                f,
+                "Failed to check number of atoms in trajectory while creating iterator"
+            ),
         }
     }
 }
@@ -234,8 +240,8 @@ pub type Result<T, E = Error> = std::result::Result<T, E>;
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::ffi::NulError;
     use std::ffi::CString;
+    use std::ffi::NulError;
 
     #[test]
     fn test_is_eof() {
@@ -293,12 +299,18 @@ mod tests {
 
         let path = Path::new(".");
         let mode = FileMode::Read;
-        let expected = Error::CouldNotOpen{path: path.to_path_buf(), mode: mode.to_owned()};
+        let expected = Error::CouldNotOpen {
+            path: path.to_path_buf(),
+            mode: mode.to_owned(),
+        };
         let err = Error::from((path, mode));
         assert_eq!(expected, err);
 
         let frame = Frame::with_capacity(0);
-        let expected = Error::WrongSizeFrame{expected: 10, found: 0};
+        let expected = Error::WrongSizeFrame {
+            expected: 10,
+            found: 0,
+        };
         let err = Error::from((&frame, 10));
         assert_eq!(expected, err);
     }
diff --git a/src/iterator.rs b/src/iterator.rs
index 1bea8cc..71b3657 100644
--- a/src/iterator.rs
+++ b/src/iterator.rs
@@ -45,24 +45,13 @@ pub struct TrajectoryIterator<T> {
     num_atoms: Result<u32>,
 }
 
-impl<T> Iterator for TrajectoryIterator<T>
-where
-    T: Trajectory,
-{
-    type Item = Result<Rc<Frame>>;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        if self.has_error {
-            return None;
-        }
-
+impl<T: Trajectory> TrajectoryIterator<T> {
+    /// Inner function for `next()`  to seperate error handling from iteration logic
+    fn next_inner(&mut self) -> <Self as Iterator>::Item {
         // If we couldn't read the number of frames when we called into_iter, return that error now
         let num_atoms = match &self.num_atoms {
             &Ok(n) => n,
-            Err(e) => {
-                self.has_error = true;
-                return Some(Err(e.clone()));
-            }
+            Err(e) => Err(Error::CheckNAtomsDuringIter(Box::new(e.clone())))?,
         };
 
         // Reuse old frame
@@ -75,8 +64,24 @@ where
             }
         };
 
-        match self.trajectory.read(item) {
-            Ok(()) => Some(Ok(Rc::clone(&self.item))),
+        self.trajectory.read(item)?;
+        Ok(Rc::clone(&self.item))
+    }
+}
+
+impl<T> Iterator for TrajectoryIterator<T>
+where
+    T: Trajectory,
+{
+    type Item = Result<Rc<Frame>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.has_error {
+            return None;
+        }
+
+        match self.next_inner() {
+            Ok(item) => Some(Ok(item)),
             Err(e) if e.is_eof() => None,
             Err(e) => {
                 self.has_error = true;
diff --git a/src/lib.rs b/src/lib.rs
index cbd1fe4..83fc2dc 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -216,9 +216,9 @@ impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
 
-        let num_atoms = self
-            .get_num_atoms()
-            .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
+        let num_atoms =
+            self.get_num_atoms()
+                .map_err(|e| Error::CheckNAtomsDuringRead(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             Err((&*frame, num_atoms))?;
         }
@@ -335,9 +335,9 @@ impl Trajectory for TRRTrajectory {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
 
-        let num_atoms = self
-            .get_num_atoms()
-            .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
+        let num_atoms =
+            self.get_num_atoms()
+                .map_err(|e| Error::CheckNAtomsDuringRead(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             Err((&*frame, num_atoms))?;
         }

From 90c4c73887f785577a20048cb44c2ac32d39e98b Mon Sep 17 00:00:00 2001
From: Josh Mitchell <yoshanuikabundi@gmail.com>
Date: Wed, 11 Nov 2020 19:19:44 +1100
Subject: [PATCH 23/24] Used cached result of self.trajectory.get_num_atoms()
 rather than storing it in TrajectoryIterator

---
 src/iterator.rs | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/iterator.rs b/src/iterator.rs
index 71b3657..540a950 100644
--- a/src/iterator.rs
+++ b/src/iterator.rs
@@ -11,7 +11,6 @@ fn into_iter_inner<T: Trajectory>(mut traj: T) -> TrajectoryIterator<T> {
         trajectory: traj,
         item: Rc::new(frame),
         has_error: false,
-        num_atoms,
     }
 }
 
@@ -42,14 +41,14 @@ pub struct TrajectoryIterator<T> {
     trajectory: T,
     item: Rc<Frame>,
     has_error: bool,
-    num_atoms: Result<u32>,
 }
 
 impl<T: Trajectory> TrajectoryIterator<T> {
     /// Inner function for `next()`  to seperate error handling from iteration logic
     fn next_inner(&mut self) -> <Self as Iterator>::Item {
         // If we couldn't read the number of frames when we called into_iter, return that error now
-        let num_atoms = match &self.num_atoms {
+        // It's OK to do this every frame because the result is cached by Trajectory
+        let num_atoms = match &self.trajectory.get_num_atoms() {
             &Ok(n) => n,
             Err(e) => Err(Error::CheckNAtomsDuringIter(Box::new(e.clone())))?,
         };

From 80b6cafc66c28050c4fbd3a8114d42a88f154fc0 Mon Sep 17 00:00:00 2001
From: daniel <bauer@cbs.tu-darmstadt.de>
Date: Thu, 12 Nov 2020 09:59:59 +0100
Subject: [PATCH 24/24] reverted to single ReadNAtom error type

---
 src/errors.rs   | 15 +++++----------
 src/iterator.rs |  2 +-
 src/lib.rs      |  4 ++--
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 1e148cd..e874f48 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -26,8 +26,7 @@ pub enum Error {
     InvalidOsStr,
     /// A path could not be converted to &CStr because it had a null byte
     NullInStr(std::ffi::NulError),
-    CheckNAtomsDuringRead(Box<Error>),
-    CheckNAtomsDuringIter(Box<Error>),
+    CouldNotCheckNAtoms(Box<Error>),
 }
 
 impl Error {
@@ -64,7 +63,7 @@ impl std::error::Error for Error {
         use Error::*;
         match &self {
             NullInStr(err) => Some(err),
-            CheckNAtomsDuringRead(err) | CheckNAtomsDuringIter(err) => Some(err.as_ref()),
+            CouldNotCheckNAtoms(err) => Some(err.as_ref()),
             _ => None,
         }
     }
@@ -123,14 +122,10 @@ 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"),
-            CheckNAtomsDuringRead(_err) => write!(
+            CouldNotCheckNAtoms(_err) => write!(
                 f,
-                "Failed to check number of atoms in trajectory while reading a frame"
-            ),
-            CheckNAtomsDuringIter(_err) => write!(
-                f,
-                "Failed to check number of atoms in trajectory while creating iterator"
-            ),
+                "Failed to read number of atoms in trajectory file"
+            )
         }
     }
 }
diff --git a/src/iterator.rs b/src/iterator.rs
index 540a950..48ee466 100644
--- a/src/iterator.rs
+++ b/src/iterator.rs
@@ -50,7 +50,7 @@ impl<T: Trajectory> TrajectoryIterator<T> {
         // It's OK to do this every frame because the result is cached by Trajectory
         let num_atoms = match &self.trajectory.get_num_atoms() {
             &Ok(n) => n,
-            Err(e) => Err(Error::CheckNAtomsDuringIter(Box::new(e.clone())))?,
+            Err(e) => Err(Error::CouldNotCheckNAtoms(Box::new(e.clone())))?,
         };
 
         // Reuse old frame
diff --git a/src/lib.rs b/src/lib.rs
index 83fc2dc..ebaf74a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -218,7 +218,7 @@ impl Trajectory for XTCTrajectory {
 
         let num_atoms =
             self.get_num_atoms()
-                .map_err(|e| Error::CheckNAtomsDuringRead(Box::new(e)))? as usize;
+                .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             Err((&*frame, num_atoms))?;
         }
@@ -337,7 +337,7 @@ impl Trajectory for TRRTrajectory {
 
         let num_atoms =
             self.get_num_atoms()
-                .map_err(|e| Error::CheckNAtomsDuringRead(Box::new(e)))? as usize;
+                .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize;
         if num_atoms != frame.coords.len() {
             Err((&*frame, num_atoms))?;
         }