From: Ben Pfaff <blp@cs.stanford.edu>
Date: Thu, 18 Dec 2025 22:16:39 +0000 (-0800)
Subject: cleanup
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7fe89fd381eb196cc67bb127435e62b3715de195;p=pspp

cleanup
---

diff --git a/rust/pspp/src/cli/convert.rs b/rust/pspp/src/cli/convert.rs
index 312268aa3a..e124c3bbc7 100644
--- a/rust/pspp/src/cli/convert.rs
+++ b/rust/pspp/src/cli/convert.rs
@@ -143,7 +143,7 @@ impl Convert {
                 self.write_data(dictionary, cases)
             }
             Some(FileType::Viewer { .. }) => {
-                let (items, page_setup) = pspp::spv::ReadOptions::new()
+                let (items, page_setup) = pspp::spv::ReadOptions::new(|e| eprintln!("{e}"))
                     .with_password(self.password.clone())
                     .open_file(&self.input)?
                     .into_parts();
diff --git a/rust/pspp/src/cli/show_spv.rs b/rust/pspp/src/cli/show_spv.rs
index aae727854a..547aed110f 100644
--- a/rust/pspp/src/cli/show_spv.rs
+++ b/rust/pspp/src/cli/show_spv.rs
@@ -90,7 +90,7 @@ impl ShowSpv {
     pub fn run(self) -> Result<()> {
         match self.mode {
             Mode::Directory => {
-                let item = pspp::spv::ReadOptions::new()
+                let item = pspp::spv::ReadOptions::new(|e| eprintln!("{e}"))
                     .with_password(self.password)
                     .open_file(&self.input)?
                     .into_items();
@@ -101,7 +101,7 @@ impl ShowSpv {
                 Ok(())
             }
             Mode::View => {
-                let item = pspp::spv::ReadOptions::new()
+                let item = pspp::spv::ReadOptions::new(|e| eprintln!("{e}"))
                     .with_password(self.password)
                     .open_file(&self.input)?
                     .into_items();
diff --git a/rust/pspp/src/spv/read.rs b/rust/pspp/src/spv/read.rs
index be3ba6ce4e..158e31c10b 100644
--- a/rust/pspp/src/spv/read.rs
+++ b/rust/pspp/src/spv/read.rs
@@ -15,9 +15,12 @@
 // this program.  If not, see <http://www.gnu.org/licenses/>.
 #![allow(dead_code)]
 use std::{
+    cell::RefCell,
+    fmt::Display,
     fs::File,
     io::{BufReader, Cursor, Read, Seek},
     path::Path,
+    rc::Rc,
 };
 
 use anyhow::{Context, anyhow};
@@ -48,9 +51,39 @@ mod legacy_bin;
 mod legacy_xml;
 mod light;
 
+/// A warning encountered reading an SPV file.
+#[derive(Clone, Debug)]
+pub struct Warning {
+    /// The name of the .zip file member inside the system file.
+    pub member: String,
+    /// Detailed warning message.
+    pub details: WarningDetails,
+}
+
+impl Display for Warning {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Warning reading member {:?}: {}",
+            &self.member, &self.details
+        )
+    }
+}
+
+/// Details of a [Warning].
+#[derive(Clone, Debug, thiserror::Error)]
+pub enum WarningDetails {
+    /// Warning reading a light detail member.
+    #[error("{0}")]
+    LightWarning(LightWarning),
+}
+
 /// Options for reading an SPV file.
-#[derive(Clone, Debug, Default)]
-pub struct ReadOptions {
+#[derive(Clone, Debug)]
+pub struct ReadOptions<F> {
+    /// Function called to report warnings.
+    pub warn: F,
+
     /// Password to use to unlock an encrypted SPV file.
     ///
     /// For an encrypted SPV file, this must be set to the (encoded or
@@ -60,18 +93,26 @@ pub struct ReadOptions {
     pub password: Option<String>,
 }
 
-impl ReadOptions {
+impl<F> ReadOptions<F> {
     /// Construct a new [ReadOptions] without a password.
-    pub fn new() -> Self {
-        Self::default()
+    pub fn new(warn: F) -> Self {
+        Self {
+            warn,
+            password: None,
+        }
     }
 
     /// Causes the file to be read by decrypting it with the given `password` or
     /// without decrypting if `password` is None.
     pub fn with_password(self, password: Option<String>) -> Self {
-        Self { password }
+        Self { password, ..self }
     }
+}
 
+impl<F> ReadOptions<F>
+where
+    F: FnMut(Warning) + 'static,
+{
     /// Opens the file at `path`.
     pub fn open_file<P>(mut self, path: P) -> Result<SpvFile, anyhow::Error>
     where
@@ -81,7 +122,7 @@ impl ReadOptions {
         if let Some(password) = self.password.take() {
             self.open_reader_encrypted(file, password)
         } else {
-            Self::open_reader_inner(file)
+            Self::open_reader_inner(file, self.warn)
         }
     }
 
@@ -94,6 +135,7 @@ impl ReadOptions {
             EncryptedFile::new(reader)?
                 .unlock(password.as_bytes())
                 .map_err(|_| anyhow!("Incorrect password."))?,
+            self.warn,
         )
     }
 
@@ -105,11 +147,11 @@ impl ReadOptions {
         if let Some(password) = self.password.take() {
             self.open_reader_encrypted(reader, password)
         } else {
-            Self::open_reader_inner(reader)
+            Self::open_reader_inner(reader, self.warn)
         }
     }
 
-    fn open_reader_inner<R>(reader: R) -> Result<SpvFile, anyhow::Error>
+    fn open_reader_inner<R>(reader: R, warn: F) -> Result<SpvFile, anyhow::Error>
     where
         R: Read + Seek + 'static,
     {
@@ -118,10 +160,10 @@ impl ReadOptions {
             ZipError::InvalidArchive(_) => Error::NotSpv,
             other => other.into(),
         })?;
-        Ok(Self::from_spv_zip_archive(&mut archive)?)
+        Ok(Self::from_spv_zip_archive(&mut archive, warn)?)
     }
 
-    fn from_spv_zip_archive<R>(archive: &mut ZipArchive<R>) -> Result<SpvFile, Error>
+    fn from_spv_zip_archive<R>(archive: &mut ZipArchive<R>, warn: F) -> Result<SpvFile, Error>
     where
         R: Read + Seek,
     {
@@ -136,12 +178,14 @@ impl ReadOptions {
         }
         drop(file);
 
+        // Read all the items.
+        let warn = Rc::new(RefCell::new(Box::new(warn) as Box<dyn FnMut(Warning)>));
         let mut items = Vec::new();
         let mut page_setup = None;
         for i in 0..archive.len() {
             let name = String::from(archive.name_for_index(i).unwrap());
             if name.starts_with("outputViewer") && name.ends_with(".xml") {
-                let (mut new_items, ps) = read_heading(archive, i, &name)?;
+                let (mut new_items, ps) = read_heading(archive, i, &name, &warn)?;
                 items.append(&mut new_items);
                 page_setup = page_setup.or(ps);
             }
@@ -207,6 +251,7 @@ fn read_heading<R>(
     archive: &mut ZipArchive<R>,
     file_number: usize,
     structure_member: &str,
+    warn: &Rc<RefCell<Box<dyn FnMut(Warning)>>>,
 ) -> Result<(Vec<Item>, Option<page::PageSetup>), Error>
 where
     R: Read + Seek,
@@ -221,7 +266,7 @@ where
         Err(error) => panic!("{error:?}"),
     };
     let page_setup = heading.page_setup.take().map(|ps| ps.decode());
-    Ok((heading.decode(archive, structure_member)?, page_setup))
+    Ok((heading.decode(archive, structure_member, warn)?, page_setup))
 }
 
 #[derive(Deserialize, Debug)]
@@ -244,6 +289,7 @@ impl Heading {
         self,
         archive: &mut ZipArchive<R>,
         structure_member: &str,
+        warn: &Rc<RefCell<Box<dyn FnMut(Warning)>>>,
     ) -> Result<Vec<Item>, Error>
     where
         R: Read + Seek,
@@ -261,7 +307,7 @@ impl Heading {
                     }
                     let item = match container.content {
                         ContainerContent::Table(table) => table
-                            .decode(archive, structure_member)
+                            .decode(archive, structure_member, warn)
                             .unwrap_or_else(|error| {
                                 new_error_item(format!("Error reading table: {error}"))
                                     .with_spv_info(SpvInfo::new(structure_member).with_error())
@@ -303,7 +349,7 @@ impl Heading {
                     let command_name = heading.command_name.take();
                     items.push(
                         heading
-                            .decode(archive, structure_member)?
+                            .decode(archive, structure_member, warn)?
                             .into_iter()
                             .collect::<Item>()
                             .with_show(show)
@@ -662,30 +708,42 @@ struct Table {
 }
 
 impl Table {
-    fn decode<R>(&self, archive: &mut ZipArchive<R>, structure_member: &str) -> Result<Item, Error>
+    fn decode<R>(
+        &self,
+        archive: &mut ZipArchive<R>,
+        structure_member: &str,
+        warn: &Rc<RefCell<Box<dyn FnMut(Warning)>>>,
+    ) -> Result<Item, Error>
     where
         R: Read + Seek,
     {
         match &self.table_structure.path {
             None => {
-                let member_name = &self.table_structure.data_path;
-                let mut light = archive.by_name(member_name)?;
+                let member_name = self.table_structure.data_path.clone();
+                let mut light = archive.by_name(&member_name)?;
                 let mut data = Vec::with_capacity(light.size() as usize);
                 light.read_to_end(&mut data)?;
                 let mut cursor = Cursor::new(data);
 
-                let table = LightTable::read_args(
-                    &mut cursor,
-                    (Box::new(|warning| println!("{warning}")),), /*XXX*/
-                )
-                .map_err(|e| {
-                    e.with_message(format!(
-                        "While parsing {member_name:?} as light binary SPV member"
-                    ))
-                })?;
-                let mut b =
-                    (Box::new(|warning| println!("{warning}")) as Box<dyn FnMut(LightWarning)>,);
-                let pivot_table = table.decode(&mut b.0);
+                let warn = warn.clone();
+                let warning = Rc::new(RefCell::new(Box::new({
+                    let warn = warn.clone();
+                    let member_name = member_name.clone();
+                    move |w| {
+                        (warn.borrow_mut())(Warning {
+                            member: member_name.clone(),
+                            details: WarningDetails::LightWarning(w),
+                        })
+                    }
+                })
+                    as Box<dyn FnMut(LightWarning)>));
+                let table =
+                    LightTable::read_args(&mut cursor, (warning.clone(),)).map_err(|e| {
+                        e.with_message(format!(
+                            "While parsing {member_name:?} as light binary SPV member"
+                        ))
+                    })?;
+                let pivot_table = table.decode(&mut *warning.borrow_mut());
                 Ok(pivot_table.into_item().with_spv_info(
                     SpvInfo::new(structure_member)
                         .with_members(SpvMembers::Light(self.table_structure.data_path.clone())),
@@ -781,7 +839,7 @@ struct TableStructure {
 #[cfg(test)]
 #[test]
 fn test_spv() {
-    let items = ReadOptions::new()
+    let items = ReadOptions::new(|e| println!("{e}"))
         .open_file("/home/blp/pspp/rust/tests/utilities/regress.spv")
         .unwrap()
         .into_items();
diff --git a/rust/pspp/src/spv/read/html.rs b/rust/pspp/src/spv/read/html.rs
index 1793965842..cbed0ddc07 100644
--- a/rust/pspp/src/spv/read/html.rs
+++ b/rust/pspp/src/spv/read/html.rs
@@ -302,8 +302,9 @@ impl Markup {
         writer
             .create_element("html")
             .write_inner_content(|w| self.write_html(w))
-            .unwrap();
-        String::from_utf8(writer.into_inner().into_inner()).unwrap()
+            .expect("writing to a Vec can't fail");
+        String::from_utf8(writer.into_inner().into_inner())
+            .expect("XmlWriter should only output UTF-8")
     }
 
     /// Returns this markup as text and attributes suitable for passing as the
@@ -426,30 +427,32 @@ impl Block {
     pub fn into_value(self) -> Value {
         let mut font_style = FontStyle::default().with_size(10);
         let cell_style = CellStyle::default().with_horz_align(Some(self.horz_align));
-        let mut markup = self.markup;
         let mut strike = false;
-        while markup.is_style() {
-            let (style, child) = markup.into_style().unwrap();
-            match style {
-                Style::Bold => font_style.bold = true,
-                Style::Italic => font_style.italic = true,
-                Style::Underline => font_style.underline = true,
-                Style::Strike => strike = true,
-                Style::Emphasis => font_style.italic = true,
-                Style::Strong => font_style.bold = true,
-                Style::Face(face) => font_style.font = face,
-                Style::Color(color) => font_style.fg = color,
-                Style::Size(points) => font_style.size = points as i32,
-            };
-            markup = child;
-        }
+        let mut markup = self.markup;
+        let mut markup = loop {
+            if let Markup::Style { style, child } = markup {
+                match style {
+                    Style::Bold => font_style.bold = true,
+                    Style::Italic => font_style.italic = true,
+                    Style::Underline => font_style.underline = true,
+                    Style::Strike => strike = true,
+                    Style::Emphasis => font_style.italic = true,
+                    Style::Strong => font_style.bold = true,
+                    Style::Face(face) => font_style.font = face,
+                    Style::Color(color) => font_style.fg = color,
+                    Style::Size(points) => font_style.size = points as i32,
+                };
+                markup = *child;
+            } else {
+                break markup;
+            }
+        };
         if strike {
             apply_style(&mut markup, Style::Strike);
         }
-        if markup.is_text() {
-            Value::new_user_text(markup.into_text().unwrap())
-        } else {
-            Value::new_markup(markup)
+        match markup {
+            Markup::Text(text) => Value::new_user_text(text),
+            markup => Value::new_markup(markup),
         }
         .with_font_style(font_style)
         .with_cell_style(cell_style)
@@ -516,20 +519,20 @@ impl Document {
             .write_inner_content(|w| {
                 for block in &self.0 {
                     w.create_element("p")
-                        .with_attribute(("align", block.horz_align.as_str().unwrap()))
+                        .with_attribute(("align", block.horz_align.as_str().unwrap_or("right")))
                         .write_inner_content(|w| block.markup.write_html(w))?;
                 }
                 Ok(())
             })
-            .unwrap();
+            .expect("writing to a Vec can't fail");
 
         // Return the result with `<html>` and `</html>` stripped off.
         str::from_utf8(&writer.into_inner().into_inner())
-            .unwrap()
+            .expect("XmlWriter should only output UTF-8")
             .strip_prefix("<html>")
-            .unwrap()
+            .expect("<html> should always be present")
             .strip_suffix("</html>")
-            .unwrap()
+            .expect("</html> should always be present")
             .into()
     }
 
@@ -701,7 +704,9 @@ fn parse_nodes(nodes: &[Node]) -> Markup {
             dst.push(markup);
         }
 
-        if let Some(mut expansion) = dst.last().unwrap().parse_variables() {
+        if let Some(last) = dst.last()
+            && let Some(mut expansion) = last.parse_variables()
+        {
             dst.pop();
             dst.append(&mut expansion);
         }
diff --git a/rust/pspp/src/spv/read/light.rs b/rust/pspp/src/spv/read/light.rs
index a8231d102e..aa9194aa3e 100644
--- a/rust/pspp/src/spv/read/light.rs
+++ b/rust/pspp/src/spv/read/light.rs
@@ -16,6 +16,7 @@ use chrono::DateTime;
 use displaydoc::Display;
 use encoding_rs::{Encoding, WINDOWS_1252};
 use enum_map::{EnumMap, enum_map};
+use itertools::Itertools;
 
 use crate::{
     data::Datum,
@@ -37,7 +38,7 @@ use crate::{
 };
 
 /// A warning decoding a light detail member.
-#[derive(Debug, Display, thiserror::Error)]
+#[derive(Clone, Debug, Display, thiserror::Error)]
 pub enum LightWarning {
     /// Unknown encoding {0:?}.
     UnknownEncoding(String),
@@ -81,11 +82,8 @@ struct Context {
 }
 
 impl Context {
-    fn new(version: Version, warn: Box<dyn FnMut(LightWarning)>) -> Self {
-        Self {
-            version: version,
-            warn: Rc::new(RefCell::new(Box::new(warn))),
-        }
+    fn new(version: Version, warn: Rc<RefCell<Box<dyn FnMut(LightWarning)>>>) -> Self {
+        Self { version, warn }
     }
     fn warn(&self, warning: LightWarning) {
         (self.warn.borrow_mut())(warning);
@@ -93,7 +91,7 @@ impl Context {
 }
 
 #[binread]
-#[br(little, import(warn: Box<dyn FnMut(LightWarning)>))]
+#[br(little, import(warn: Rc<RefCell<Box<dyn FnMut(LightWarning)>>>))]
 #[derive(Debug)]
 pub struct LightTable {
     header: Header,
@@ -1736,11 +1734,7 @@ impl Axes {
             }
             axes[index] = Some(axis);
         }
-        Ok(axes
-            .into_iter()
-            .map(|axis| axis.unwrap())
-            .zip(dimensions)
-            .collect())
+        Ok(axes.into_iter().flatten().zip_eq(dimensions).collect())
     }
 }
 
diff --git a/rust/pspp/src/sys/raw.rs b/rust/pspp/src/sys/raw.rs
index 4519bf2de5..862d5a81fe 100644
--- a/rust/pspp/src/sys/raw.rs
+++ b/rust/pspp/src/sys/raw.rs
@@ -71,6 +71,7 @@ use std::{
     mem::take,
     num::NonZeroU8,
     ops::Range,
+    sync::Arc,
 };
 use thiserror::Error as ThisError;
 
@@ -79,7 +80,7 @@ pub mod records;
 /// An error encountered reading raw system file records.
 ///
 /// Any error prevents reading further data from the system file.
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct Error<D> {
     /// Range of file offsets where the error occurred.
     pub offsets: Option<Range<u64>>,
@@ -129,7 +130,7 @@ where
 }
 
 /// Details of an [Error].
-#[derive(ThisError, Debug)]
+#[derive(Clone, ThisError, Debug)]
 pub enum ErrorDetails {
     /// Not an SPSS system file.
     #[error("Not an SPSS system file")]
@@ -145,7 +146,7 @@ pub enum ErrorDetails {
 
     /// I/O error.
     #[error("I/O error ({0})")]
-    Io(#[from] IoError),
+    Io(Arc<IoError>),
 
     /// Invalid SAV compression code.
     #[error("Invalid SAV compression code {0}")]
@@ -249,6 +250,12 @@ pub enum ErrorDetails {
     ),
 }
 
+impl From<IoError> for ErrorDetails {
+    fn from(value: IoError) -> Self {
+        ErrorDetails::Io(Arc::new(value))
+    }
+}
+
 /// A warning reading a raw system file record.
 ///
 /// Warnings indicate that something may be amiss, but they do not prevent
@@ -1116,7 +1123,7 @@ impl CompressionAction {
 /// An error reading a case from a system file.
 ///
 /// Used for SPSS system files and SPSS/PC+ system files.
-#[derive(ThisError, Display, Debug)]
+#[derive(Clone, ThisError, Display, Debug)]
 pub enum CaseDetails {
     /// Unexpected end of file {case_ofs} bytes into case {case_number} with expected length {case_len} bytes.
     EofInCase {
@@ -1147,7 +1154,13 @@ pub enum CaseDetails {
     },
 
     /// I/O error ({0})
-    Io(#[from] IoError),
+    Io(Arc<IoError>),
+}
+
+impl From<IoError> for CaseDetails {
+    fn from(value: IoError) -> Self {
+        CaseDetails::Io(Arc::new(value))
+    }
 }
 
 impl Datum<ByteString> {
diff --git a/rust/pspp/src/sys/raw/records.rs b/rust/pspp/src/sys/raw/records.rs
index 3fae0faa2a..791698caf6 100644
--- a/rust/pspp/src/sys/raw/records.rs
+++ b/rust/pspp/src/sys/raw/records.rs
@@ -9,6 +9,7 @@ use std::{
     io::{Cursor, ErrorKind, Read, Seek, SeekFrom},
     ops::Range,
     str::from_utf8,
+    sync::Arc,
 };
 
 use crate::{
@@ -2520,11 +2521,11 @@ impl ZHeader {
 }
 
 /// Error reading a [ZHeader].
-#[derive(ThisError, Debug)]
+#[derive(Clone, ThisError, Debug)]
 pub enum ZHeaderError {
     /// I/O error via [mod@binrw].
     #[error("{}", DisplayBinError(.0, "ZLIB header"))]
-    BinError(#[from] BinError),
+    BinError(Arc<BinError>),
 
     /// Impossible ztrailer_offset {0:#x}.
     #[error("Impossible ztrailer_offset {0:#x}.")]
@@ -2550,6 +2551,12 @@ pub enum ZHeaderError {
     ),
 }
 
+impl From<BinError> for ZHeaderError {
+    fn from(value: BinError) -> Self {
+        ZHeaderError::BinError(Arc::new(value))
+    }
+}
+
 /// A ZLIB trailer in a system file.
 #[derive(Clone, Debug, Serialize)]
 pub struct ZTrailer {
@@ -2668,11 +2675,11 @@ impl<'a> Display for DisplayBinError<'a> {
 }
 
 /// Error reading a [ZTrailer].
-#[derive(ThisError, Debug)]
+#[derive(Clone, ThisError, Debug)]
 pub enum ZTrailerError {
     /// I/O error via [mod@binrw].
     #[error("{}", DisplayBinError(.0, "ZLIB trailer"))]
-    BinError(#[from] BinError),
+    BinError(Arc<BinError>),
 
     /// ZLIB trailer bias {actual} is not {} as expected from file header bias.
     #[
@@ -2769,6 +2776,12 @@ pub enum ZTrailerError {
     },
 }
 
+impl From<BinError> for ZTrailerError {
+    fn from(value: BinError) -> Self {
+        ZTrailerError::BinError(Arc::new(value))
+    }
+}
+
 impl ZTrailer {
     /// Reads a ZLIB trailer from `reader` using `endian`.  `bias` is the
     /// floating-point bias for confirmation against the trailer, and `zheader`