Code review of src/pdata/array*

src/pdata/array.rs

@@ -1,39 +0,0 @@
-use nom::{number::complete::*, IResult};
-use std::fmt;
-
-use crate::pdata::unpack::*;
-
-//------------------------------------------
-
-// TODO: build a data structure representating an array?
-
-// FIXME: rename this struct
-pub struct ArrayBlockEntry {
-    pub block: u64,
-}
-
-impl Unpack for ArrayBlockEntry {
-    fn disk_size() -> u32 {
-        8
-    }
-
-    fn unpack(i: &[u8]) -> IResult<&[u8], ArrayBlockEntry> {
-        let (i, n) = le_u64(i)?;
-        let block = n;
-
-        Ok((
-            i,
-            ArrayBlockEntry {
-                block,
-            }
-        ))
-    }
-}
-
-impl fmt::Display for ArrayBlockEntry {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.block)
-    }
-}
-
-//------------------------------------------

src/pdata/array_block.rs

@@ -33,7 +33,7 @@ impl Unpack for ArrayBlockHeader {
                 max_entries,
                 nr_entries,
                 value_size,
-                blocknr
+                blocknr,
             },
         ))
     }
@@ -44,27 +44,17 @@ pub struct ArrayBlock<V: Unpack> {
     pub values: Vec<V>,
 }
 
-impl<V: Unpack> ArrayBlock<V> {
-    pub fn get_header(&self) -> &ArrayBlockHeader {
-        &self.header
-    }
-}
-
 fn convert_result<V>(r: IResult<&[u8], V>) -> Result<(&[u8], V)> {
     r.map_err(|_| anyhow!("parse error"))
 }
 
-pub fn unpack_array_block<V: Unpack>(
+pub fn unpack_array_block<V: Unpack>(data: &[u8]) -> Result<ArrayBlock<V>> {
-    data: &[u8],
-) -> Result<ArrayBlock<V>> {
     // TODO: collect errors
-    let (i, header) = ArrayBlockHeader::unpack(data).map_err(|_e| anyhow!("Couldn't parse header"))?;
+    let (i, header) =
+        ArrayBlockHeader::unpack(data).map_err(|_e| anyhow!("Couldn't parse header"))?;
     let (_i, values) = convert_result(count(V::unpack, header.nr_entries as usize)(i))?;
 
-    Ok(ArrayBlock {
+    Ok(ArrayBlock { header, values })
-        header,
-        values,
-    })
 }
 
 //------------------------------------------

src/pdata/array_walker.rs

@@ -1,7 +1,6 @@
 use std::sync::Arc;
 
 use crate::io_engine::*;
-use crate::pdata::array::*;
 use crate::pdata::array_block::*;
 use crate::pdata::btree::*;
 use crate::pdata::btree_walker::*;
@@ -11,16 +10,12 @@ use crate::pdata::unpack::*;
 
 pub struct ArrayWalker {
     engine: Arc<dyn IoEngine + Send + Sync>,
-    ignore_non_fatal: bool
+    ignore_non_fatal: bool,
 }
 
 // FIXME: define another Result type for array visiting?
 pub trait ArrayBlockVisitor<V: Unpack> {
-    fn visit(
+    fn visit(&self, index: u64, v: V) -> anyhow::Result<()>;
-        &self,
-        index: u64,
-        v: V,
-    ) -> anyhow::Result<()>;
 }
 
 struct BlockValueVisitor<V> {
@@ -29,9 +24,9 @@ struct BlockValueVisitor<V> {
 }
 
 impl<V: Unpack + Copy> BlockValueVisitor<V> {
-    pub fn new<Visitor: 'static + ArrayBlockVisitor<V>>(
+    pub fn new(
         e: Arc<dyn IoEngine + Send + Sync>,
-        v: Box<Visitor>,
+        v: Box<dyn ArrayBlockVisitor<V>>,
     ) -> BlockValueVisitor<V> {
         BlockValueVisitor {
             engine: e,
@@ -39,19 +34,17 @@ impl<V: Unpack + Copy> BlockValueVisitor<V> {
         }
     }
 
-    pub fn visit_array_block(
+    pub fn visit_array_block(&self, index: u64, array_block: ArrayBlock<V>) {
-        &self,
+        let begin = index * array_block.header.max_entries as u64;
-        index: u64,
-        array_block: ArrayBlock<V>,
-    ) {
-        let begin = index * u64::from(array_block.header.nr_entries);
         for i in 0..array_block.header.nr_entries {
-            self.array_block_visitor.visit(begin + u64::from(i), array_block.values[i as usize]).unwrap();
+            self.array_block_visitor
+                .visit(begin + i as u64, array_block.values[i as usize])
+                .unwrap();
         }
     }
 }
 
-impl<V: Unpack + Copy> NodeVisitor<ArrayBlockEntry> for BlockValueVisitor<V> {
+impl<V: Unpack + Copy> NodeVisitor<u64> for BlockValueVisitor<V> {
     // FIXME: return errors
     fn visit(
         &self,
@@ -59,23 +52,20 @@ impl<V: Unpack + Copy> NodeVisitor<ArrayBlockEntry> for BlockValueVisitor<V> {
         _kr: &KeyRange,
         _h: &NodeHeader,
         keys: &[u64],
-        values: &[ArrayBlockEntry],
+        values: &[u64],
     ) -> Result<()> {
-        for n in 0..keys.len() {
+        for (n, index) in keys.iter().enumerate() {
-            let index = keys[n];
+            let b = self.engine.read(values[n]).map_err(|_| io_err(path))?;
-            let b = self.engine.read(values[n].block).map_err(|_| io_err(path))?;
             let array_block = unpack_array_block::<V>(b.get_data()).map_err(|_| io_err(path))?;
-            self.visit_array_block(index, array_block);
+            self.visit_array_block(*index, array_block);
         }
         Ok(())
     }
 
-    // FIXME: stub
     fn visit_again(&self, _path: &[u64], _b: u64) -> Result<()> {
         Ok(())
     }
 
-    // FIXME: stub
     fn end_walk(&self) -> Result<()> {
         Ok(())
     }
@@ -90,15 +80,13 @@ impl ArrayWalker {
         r
     }
 
-    // FIXME: pass the visitor by reference?
     // FIXME: redefine the Result type for array visiting?
-    pub fn walk<BV, V: Copy>(&self, visitor: Box<BV>, root: u64) -> Result<()>
+    pub fn walk<V>(&self, visitor: Box<dyn ArrayBlockVisitor<V>>, root: u64) -> Result<()>
     where
-        BV: 'static + ArrayBlockVisitor<V>,
+        V: Unpack + Copy,
-        V: Unpack,
     {
         let w = BTreeWalker::new(self.engine.clone(), self.ignore_non_fatal);
-        let mut path = Vec::new(); // FIXME: eliminate this line?
+        let mut path = Vec::new();
         path.push(0);
         let v = BlockValueVisitor::<V>::new(self.engine.clone(), visitor);
         w.walk(&mut path, &v, root)

src/pdata/mod.rs

@@ -1,4 +1,3 @@
-pub mod array;
 pub mod array_block;
 pub mod array_walker;
 pub mod btree;