From 917d649edcdac473d71f08cfc360ebcf12c8bcf7 Mon Sep 17 00:00:00 2001 From: David Laban Date: Sat, 8 Jun 2019 22:50:09 +0100 Subject: [PATCH 1/4] refactor impl Display for Table This passes refs to table and path around, and avoids the use of Cell and RefCell. It splits the job of walking the tree from that of printing bytes by using the visitor pattern. This is probably net-neutral for readability, but it allows us to extend the formatting logic later if we need to. --- src/display.rs | 96 +++++++++++++++++++++++++++----------------------- 1 file changed, 51 insertions(+), 45 deletions(-) diff --git a/src/display.rs b/src/display.rs index ac9f92fc..43f0021f 100644 --- a/src/display.rs +++ b/src/display.rs @@ -78,61 +78,67 @@ impl Display for InlineTable { } } -struct TableFormatState<'t> { - path: RefCell>, - table: Cell<&'t Table>, -} - -impl<'a> Display for TableFormatState<'a> { - fn fmt(&self, f: &mut Formatter) -> Result { - let table = self.table.get(); +fn visit_nested_tables<'t, F>( + table: &'t Table, + path: &mut Vec<&'t str>, + is_array_of_tables: bool, + callback: &mut F, +) -> Result +where + F: FnMut(&Table, &Vec<&'t str>, bool) -> Result, +{ + callback(table, path, is_array_of_tables)?; - for kv in table.items.values() { - if let Item::Value(ref value) = kv.value { - writeln!(f, "{}={}", kv.key, value)?; + for kv in table.items.values() { + match kv.value { + Item::Table(ref t) => { + path.push(&kv.key.raw_value); + visit_nested_tables(t, path, false, callback)?; + path.pop(); } - } - - for kv in table.items.values() { - match kv.value { - Item::Table(ref t) => { - self.path.borrow_mut().push(&kv.key.raw_value); - self.table.set(t); - if !(t.implicit && t.values_len() == 0) { - write!(f, "{}[", t.decor.prefix)?; - write!(f, "{}", self.path.borrow().join("."))?; - writeln!(f, "]{}", t.decor.suffix)?; - } - write!(f, "{}", self)?; - self.table.set(table); - self.path.borrow_mut().pop(); + Item::ArrayOfTables(ref a) => { + for t in a.iter() { + path.push(&kv.key.raw_value); + visit_nested_tables(t, path, true, callback)?; + path.pop(); } - Item::ArrayOfTables(ref a) => { - self.path.borrow_mut().push(&kv.key.raw_value); - for t in a.iter() { - self.table.set(t); - write!(f, "{}[[", t.decor.prefix)?; - write!(f, "{}", self.path.borrow().join("."))?; - writeln!(f, "]]{}", t.decor.suffix)?; - write!(f, "{}", self)?; - } - self.table.set(table); - self.path.borrow_mut().pop(); - } - _ => {} } + _ => {} } - Ok(()) } + Ok(()) } impl Display for Table { fn fmt(&self, f: &mut Formatter) -> Result { - let state = TableFormatState { - path: RefCell::new(Vec::new()), - table: Cell::new(self), - }; - write!(f, "{}", state) + let mut path = Vec::new(); + + visit_nested_tables( + self, + &mut path, + false, + &mut |t: &Table, path, is_array_of_tables: bool| { + if path.len() == 0 { + // don't print header for the root node + } else if is_array_of_tables { + write!(f, "{}[[", t.decor.prefix)?; + write!(f, "{}", path.join("."))?; + writeln!(f, "]]{}", t.decor.suffix)?; + } else if !(t.implicit && t.values_len() == 0) { + write!(f, "{}[", t.decor.prefix)?; + write!(f, "{}", path.join("."))?; + writeln!(f, "]{}", t.decor.suffix)?; + } + // print table body + for kv in t.items.values() { + if let Item::Value(ref value) = kv.value { + writeln!(f, "{}={}", kv.key, value)?; + } + } + Ok(()) + }, + )?; + Ok(()) } } From 77c577dfebe3e392f39db1569aba9514c4f31e0b Mon Sep 17 00:00:00 2001 From: David Laban Date: Sun, 9 Jun 2019 15:35:47 +0100 Subject: [PATCH 2/4] make visit_nested_tables a method on Table --- src/display.rs | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/display.rs b/src/display.rs index 43f0021f..ab51f65d 100644 --- a/src/display.rs +++ b/src/display.rs @@ -78,43 +78,44 @@ impl Display for InlineTable { } } -fn visit_nested_tables<'t, F>( - table: &'t Table, - path: &mut Vec<&'t str>, - is_array_of_tables: bool, - callback: &mut F, -) -> Result -where - F: FnMut(&Table, &Vec<&'t str>, bool) -> Result, -{ - callback(table, path, is_array_of_tables)?; +impl Table { + fn visit_nested_tables<'t, F>( + &'t self, + path: &mut Vec<&'t str>, + is_array_of_tables: bool, + callback: &mut F, + ) -> Result + where + F: FnMut(&Table, &Vec<&'t str>, bool) -> Result, + { + callback(self, path, is_array_of_tables)?; - for kv in table.items.values() { - match kv.value { - Item::Table(ref t) => { - path.push(&kv.key.raw_value); - visit_nested_tables(t, path, false, callback)?; - path.pop(); - } - Item::ArrayOfTables(ref a) => { - for t in a.iter() { + for kv in self.items.values() { + match kv.value { + Item::Table(ref t) => { path.push(&kv.key.raw_value); - visit_nested_tables(t, path, true, callback)?; + t.visit_nested_tables(path, false, callback)?; path.pop(); } + Item::ArrayOfTables(ref a) => { + for t in a.iter() { + path.push(&kv.key.raw_value); + t.visit_nested_tables(path, true, callback)?; + path.pop(); + } + } + _ => {} } - _ => {} } + Ok(()) } - Ok(()) } impl Display for Table { fn fmt(&self, f: &mut Formatter) -> Result { let mut path = Vec::new(); - visit_nested_tables( - self, + self.visit_nested_tables( &mut path, false, &mut |t: &Table, path, is_array_of_tables: bool| { From 1b8f02fabf849c5b705486b73ec535081021666f Mon Sep 17 00:00:00 2001 From: David Laban Date: Sun, 9 Jun 2019 16:20:57 +0100 Subject: [PATCH 3/4] make visit_table a module level function --- src/display.rs | 53 +++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/display.rs b/src/display.rs index ab51f65d..5df56749 100644 --- a/src/display.rs +++ b/src/display.rs @@ -111,34 +111,39 @@ impl Table { } } +fn visit_table( + f: &mut Formatter, + table: &Table, + path: &Vec<&str>, + is_array_of_tables: bool, +) -> Result { + if path.len() == 0 { + // don't print header for the root node + } else if is_array_of_tables { + write!(f, "{}[[", table.decor.prefix)?; + write!(f, "{}", path.join("."))?; + writeln!(f, "]]{}", table.decor.suffix)?; + } else if !(table.implicit && table.values_len() == 0) { + write!(f, "{}[", table.decor.prefix)?; + write!(f, "{}", path.join("."))?; + writeln!(f, "]{}", table.decor.suffix)?; + } + // print table body + for kv in table.items.values() { + if let Item::Value(ref value) = kv.value { + writeln!(f, "{}={}", kv.key, value)?; + } + } + Ok(()) +} + impl Display for Table { fn fmt(&self, f: &mut Formatter) -> Result { let mut path = Vec::new(); - self.visit_nested_tables( - &mut path, - false, - &mut |t: &Table, path, is_array_of_tables: bool| { - if path.len() == 0 { - // don't print header for the root node - } else if is_array_of_tables { - write!(f, "{}[[", t.decor.prefix)?; - write!(f, "{}", path.join("."))?; - writeln!(f, "]]{}", t.decor.suffix)?; - } else if !(t.implicit && t.values_len() == 0) { - write!(f, "{}[", t.decor.prefix)?; - write!(f, "{}", path.join("."))?; - writeln!(f, "]{}", t.decor.suffix)?; - } - // print table body - for kv in t.items.values() { - if let Item::Value(ref value) = kv.value { - writeln!(f, "{}={}", kv.key, value)?; - } - } - Ok(()) - }, - )?; + self.visit_nested_tables(&mut path, false, &mut |t, path, is_array| { + visit_table(f, t, path, is_array) + })?; Ok(()) } } From c0a03e2467b0fc1ec116a2e9eba359d0af50333f Mon Sep 17 00:00:00 2001 From: David Laban Date: Sun, 9 Jun 2019 23:15:38 +0100 Subject: [PATCH 4/4] clippy fixes --- src/display.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/display.rs b/src/display.rs index 5df56749..eb8adc28 100644 --- a/src/display.rs +++ b/src/display.rs @@ -2,7 +2,6 @@ use crate::decor::{Formatted, Repr}; use crate::document::Document; use crate::table::{Item, Table}; use crate::value::{Array, DateTime, InlineTable, Value}; -use std::cell::{Cell, RefCell}; use std::fmt::{Display, Formatter, Result}; impl Display for Repr { @@ -114,10 +113,10 @@ impl Table { fn visit_table( f: &mut Formatter, table: &Table, - path: &Vec<&str>, + path: &[&str], is_array_of_tables: bool, ) -> Result { - if path.len() == 0 { + if path.is_empty() { // don't print header for the root node } else if is_array_of_tables { write!(f, "{}[[", table.decor.prefix)?;