From bd626df70ee0bb0f57c8893185820d3dd51814b3 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 6 Mar 2022 23:28:05 -0500 Subject: [PATCH] dd: replace cascading if/else if with enum match Replace a cascading `if/else if` chain in `conv_block_unblock_helper()` with a match statement on a new enum, `ConversionMode`, that enumerates the various modes in which `dd` can operate. --- src/uu/dd/src/blocks.rs | 147 +++++++++++++++++--------------- src/uu/dd/src/datastructures.rs | 27 ++++-- src/uu/dd/src/dd.rs | 2 +- 3 files changed, 95 insertions(+), 81 deletions(-) diff --git a/src/uu/dd/src/blocks.rs b/src/uu/dd/src/blocks.rs index 331bad56b..292f9ce09 100644 --- a/src/uu/dd/src/blocks.rs +++ b/src/uu/dd/src/blocks.rs @@ -6,7 +6,7 @@ // spell-checker:ignore datastructures rstat rposition cflags ctable use crate::conversion_tables::ConversionTable; -use crate::datastructures::InternalError; +use crate::datastructures::ConversionMode; use crate::progress::ReadStat; use crate::Input; use std::io::Read; @@ -65,6 +65,47 @@ fn unblock(buf: &[u8], cbs: usize) -> Vec { }) } +/// Given the various command-line parameters, determine the conversion mode. +/// +/// The `conv` command-line option can take many different values, +/// each of which may combine with others. For example, `conv=ascii`, +/// `conv=lcase`, `conv=sync`, and so on. The arguments to this +/// function represent the settings of those various command-line +/// parameters. This function translates those settings to a +/// [`ConversionMode`]. +fn conversion_mode( + ctable: Option<&ConversionTable>, + block: Option, + unblock: Option, + non_ascii: bool, + is_sync: bool, +) -> Option { + match (ctable, block, unblock) { + (Some(ct), None, None) => Some(ConversionMode::ConvertOnly(ct)), + (Some(ct), Some(cbs), None) => { + if non_ascii { + Some(ConversionMode::ConvertThenBlock(ct, cbs, is_sync)) + } else { + Some(ConversionMode::BlockThenConvert(ct, cbs, is_sync)) + } + } + (Some(ct), None, Some(cbs)) => { + if non_ascii { + Some(ConversionMode::ConvertThenUnblock(ct, cbs)) + } else { + Some(ConversionMode::UnblockThenConvert(ct, cbs)) + } + } + (None, Some(cbs), None) => Some(ConversionMode::BlockOnly(cbs, is_sync)), + (None, None, Some(cbs)) => Some(ConversionMode::UnblockOnly(cbs)), + (None, None, None) => None, + // The remaining variants should never happen because the + // argument parsing above should result in an error before + // getting to this line of code. + _ => unreachable!(), + } +} + /// A helper for teasing out which options must be applied and in which order. /// Some user options, such as the presence of conversion tables, will determine whether the input is assumed to be ascii. The parser sets the Input::non_ascii flag accordingly. /// Examples: @@ -76,94 +117,58 @@ pub(crate) fn conv_block_unblock_helper( mut buf: Vec, i: &mut Input, rstat: &mut ReadStat, -) -> Result, InternalError> { - // Local Predicate Fns ------------------------------------------------- - fn should_block_then_conv(i: &Input) -> bool { - !i.non_ascii && i.cflags.block.is_some() - } - fn should_conv_then_block(i: &Input) -> bool { - i.non_ascii && i.cflags.block.is_some() - } - fn should_unblock_then_conv(i: &Input) -> bool { - !i.non_ascii && i.cflags.unblock.is_some() - } - fn should_conv_then_unblock(i: &Input) -> bool { - i.non_ascii && i.cflags.unblock.is_some() - } - fn conv_only(i: &Input) -> bool { - i.cflags.ctable.is_some() && i.cflags.block.is_none() && i.cflags.unblock.is_none() - } - // Local Helper Fns ---------------------------------------------------- +) -> Vec { + // TODO This function has a mutable input `buf` but also returns a + // completely new `Vec`; that seems fishy. Could we either make + // the input immutable or make the function not return anything? + fn apply_conversion(buf: &mut [u8], ct: &ConversionTable) { for idx in 0..buf.len() { buf[idx] = ct[buf[idx] as usize]; } } - // -------------------------------------------------------------------- - if conv_only(i) { - // no block/unblock - let ct = i.cflags.ctable.unwrap(); - apply_conversion(&mut buf, ct); - Ok(buf) - } else if should_block_then_conv(i) { - // ascii input so perform the block first - let cbs = i.cflags.block.unwrap(); - - let mut blocks = block(&buf, cbs, i.cflags.sync.is_some(), rstat); - - if let Some(ct) = i.cflags.ctable { + let mode = conversion_mode( + i.cflags.ctable, + i.cflags.block, + i.cflags.unblock, + i.non_ascii, + i.cflags.sync.is_some(), + ) + .unwrap(); + match &mode { + ConversionMode::ConvertOnly(ct) => { + apply_conversion(&mut buf, ct); + buf + } + ConversionMode::BlockThenConvert(ct, cbs, sync) => { + let mut blocks = block(&buf, *cbs, *sync, rstat); for buf in &mut blocks { apply_conversion(buf, ct); } + blocks.into_iter().flatten().collect() } - - let blocks = blocks.into_iter().flatten().collect(); - - Ok(blocks) - } else if should_conv_then_block(i) { - // Non-ascii so perform the conversion first - let cbs = i.cflags.block.unwrap(); - - if let Some(ct) = i.cflags.ctable { + ConversionMode::ConvertThenBlock(ct, cbs, sync) => { apply_conversion(&mut buf, ct); + block(&buf, *cbs, *sync, rstat) + .into_iter() + .flatten() + .collect() } - - let blocks = block(&buf, cbs, i.cflags.sync.is_some(), rstat) + ConversionMode::BlockOnly(cbs, sync) => block(&buf, *cbs, *sync, rstat) .into_iter() .flatten() - .collect(); - - Ok(blocks) - } else if should_unblock_then_conv(i) { - // ascii input so perform the unblock first - let cbs = i.cflags.unblock.unwrap(); - - let mut buf = unblock(&buf, cbs); - - if let Some(ct) = i.cflags.ctable { + .collect(), + ConversionMode::UnblockThenConvert(ct, cbs) => { + let mut buf = unblock(&buf, *cbs); apply_conversion(&mut buf, ct); + buf } - - Ok(buf) - } else if should_conv_then_unblock(i) { - // Non-ascii input so perform the conversion first - let cbs = i.cflags.unblock.unwrap(); - - if let Some(ct) = i.cflags.ctable { + ConversionMode::ConvertThenUnblock(ct, cbs) => { apply_conversion(&mut buf, ct); + unblock(&buf, *cbs) } - - let buf = unblock(&buf, cbs); - - Ok(buf) - } else { - // The following error should not happen, as it results from - // insufficient command line data. This case should be caught - // by the parser before making it this far. - // Producing this error is an alternative to risking an unwrap call - // on 'cbs' if the required data is not provided. - Err(InternalError::InvalidConvBlockUnblockCase) + ConversionMode::UnblockOnly(cbs) => unblock(&buf, *cbs), } } diff --git a/src/uu/dd/src/datastructures.rs b/src/uu/dd/src/datastructures.rs index 6529f6602..17266ada1 100644 --- a/src/uu/dd/src/datastructures.rs +++ b/src/uu/dd/src/datastructures.rs @@ -14,6 +14,23 @@ use crate::conversion_tables::*; type Cbs = usize; +/// How to apply conversion, blocking, and/or unblocking. +/// +/// Certain settings of the `conv` parameter to `dd` require a +/// combination of conversion, blocking, or unblocking, applied in a +/// certain order. The variants of this enumeration give the different +/// ways of combining those three operations. +#[derive(Debug, PartialEq)] +pub(crate) enum ConversionMode<'a> { + ConvertOnly(&'a ConversionTable), + BlockOnly(Cbs, bool), + UnblockOnly(Cbs), + BlockThenConvert(&'a ConversionTable, Cbs, bool), + ConvertThenBlock(&'a ConversionTable, Cbs, bool), + UnblockThenConvert(&'a ConversionTable, Cbs), + ConvertThenUnblock(&'a ConversionTable, Cbs), +} + /// Stores all Conv Flags that apply to the input #[derive(Debug, Default, PartialEq)] pub struct IConvFlags { @@ -91,19 +108,11 @@ pub enum CountType { pub enum InternalError { WrongInputType, WrongOutputType, - InvalidConvBlockUnblockCase, } impl std::fmt::Display for InternalError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::WrongInputType | Self::WrongOutputType => { - write!(f, "Internal dd error: Wrong Input/Output data type") - } - Self::InvalidConvBlockUnblockCase => { - write!(f, "Invalid Conversion, Block, or Unblock data") - } - } + write!(f, "Internal dd error: Wrong Input/Output data type") } } diff --git a/src/uu/dd/src/dd.rs b/src/uu/dd/src/dd.rs index cdfcdb732..d004a592d 100644 --- a/src/uu/dd/src/dd.rs +++ b/src/uu/dd/src/dd.rs @@ -640,7 +640,7 @@ fn read_helper(i: &mut Input, bsize: usize) -> std::io::Result<(Read perform_swab(&mut buf); } if is_conv(i) || is_block(i) || is_unblock(i) { - let buf = conv_block_unblock_helper(buf, i, &mut rstat).unwrap(); + let buf = conv_block_unblock_helper(buf, i, &mut rstat); Ok((rstat, buf)) } else { Ok((rstat, buf))