Repository: rust-itertools/itertools (language: rust)

Issue:
FormatWith should use a Cell
The `RefCell` is unnecessary, as all we're doing is `.take()`, which is also available on `Cell`: https://doc.rust-lang.org/stable/std/cell/struct.Cell.html#method.take

https://github.com/rust-itertools/itertools/blob/943494c2be3977a42df701dfde90d903ae17d16d/src/format.rs#L14

This results in using more memory and generating more code than necessary.


Discussion / hints:

Fix patch:
diff --git a/src/format.rs b/src/format.rs
index d87cee950..c4cb65dcb 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -1,5 +1,5 @@
+use std::cell::Cell;
 use std::fmt;
-use std::cell::RefCell;
 
 /// Format all iterator elements lazily, separated by `sep`.
 ///
@@ -7,11 +7,10 @@ use std::cell::RefCell;
 /// exhausted.
 ///
 /// See [`.format_with()`](crate::Itertools::format_with) for more information.
-#[derive(Clone)]
 pub struct FormatWith<'a, I, F> {
     sep: &'a str,
     /// FormatWith uses interior mutability because Display::fmt takes &self.
-    inner: RefCell<Option<(I, F)>>,
+    inner: Cell<Option<(I, F)>>,
 }
 
 /// Format all iterator elements lazily, separated by `sep`.
@@ -21,38 +20,40 @@ pub struct FormatWith<'a, I, F> {
 ///
 /// See [`.format()`](crate::Itertools::format)
 /// for more information.
-#[derive(Clone)]
 pub struct Format<'a, I> {
     sep: &'a str,
     /// Format uses interior mutability because Display::fmt takes &self.
-    inner: RefCell<Option<I>>,
+    inner: Cell<Option<I>>,
 }
 
 pub fn new_format<I, F>(iter: I, separator: &str, f: F) -> FormatWith<'_, I, F>
-    where I: Iterator,
-          F: FnMut(I::Item, &mut dyn FnMut(&dyn fmt::Display) -> fmt::Result) -> fmt::Result
+where
+    I: Iterator,
+    F: FnMut(I::Item, &mut dyn FnMut(&dyn fmt::Display) -> fmt::Result) -> fmt::Result,
 {
     FormatWith {
         sep: separator,
-        inner: RefCell::new(Some((iter, f))),
+        inner: Cell::new(Some((iter, f))),
     }
 }
 
 pub fn new_format_default<I>(iter: I, separator: &str) -> Format<'_, I>
-    where I: Iterator,
+where
+    I: Iterator,
 {
     Format {
         sep: separator,
-        inner: RefCell::new(Some(iter)),
+        inner: Cell::new(Some(iter)),
     }
 }
 
 impl<'a, I, F> fmt::Display for FormatWith<'a, I, F>
-    where I: Iterator,
-          F: FnMut(I::Item, &mut dyn  FnMut(&dyn fmt::Display) -> fmt::Result) -> fmt::Result
+where
+    I: Iterator,
+    F: FnMut(I::Item, &mut dyn FnMut(&dyn fmt::Display) -> fmt::Result) -> fmt::Result,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let (mut iter, mut format) = match self.inner.borrow_mut().take() {
+        let (mut iter, mut format) = match self.inner.take() {
             Some(t) => t,
             None => panic!("FormatWith: was already formatted once"),
         };
@@ -71,12 +72,15 @@ impl<'a, I, F> fmt::Display for FormatWith<'a, I, F>
 }
 
 impl<'a, I> Format<'a, I>
-    where I: Iterator,
+where
+    I: Iterator,
 {
-    fn format<F>(&self, f: &mut fmt::Formatter, mut cb: F) -> fmt::Result
-        where F: FnMut(&I::Item, &mut fmt::Formatter) -> fmt::Result,
-    {
-        let mut iter = match self.inner.borrow_mut().take() {
+    fn format(
+        &self,
+        f: &mut fmt::Formatter,
+        cb: fn(&I::Item, &mut fmt::Formatter) -> fmt::Result,
+    ) -> fmt::Result {
+        let mut iter = match self.inner.take() {
             Some(t) => t,
             None => panic!("Format: was already formatted once"),
         };
@@ -109,5 +113,56 @@ macro_rules! impl_format {
     }
 }
 
-impl_format!{Display Debug
-             UpperExp LowerExp UpperHex LowerHex Octal Binary Pointer}
+impl_format! {Display Debug UpperExp LowerExp UpperHex LowerHex Octal Binary Pointer}
+
+impl<'a, I, F> Clone for FormatWith<'a, I, F>
+where
+    (I, F): Clone,
+{
+    fn clone(&self) -> Self {
+        struct PutBackOnDrop<'r, 'a, I, F> {
+            into: &'r FormatWith<'a, I, F>,
+            inner: Option<(I, F)>,
+        }
+        // This ensures we preserve the state of the original `FormatWith` if `Clone` panics
+        impl<'r, 'a, I, F> Drop for PutBackOnDrop<'r, 'a, I, F> {
+            fn drop(&mut self) {
+                self.into.inner.set(self.inner.take())
+            }
+        }
+        let pbod = PutBackOnDrop {
+            inner: self.inner.take(),
+            into: self,
+        };
+        Self {
+            inner: Cell::new(pbod.inner.clone()),
+            sep: self.sep,
+        }
+    }
+}
+
+impl<'a, I> Clone for Format<'a, I>
+where
+    I: Clone,
+{
+    fn clone(&self) -> Self {
+        struct PutBackOnDrop<'r, 'a, I> {
+            into: &'r Format<'a, I>,
+            inner: Option<I>,
+        }
+        // This ensures we preserve the state of the original `FormatWith` if `Clone` panics
+        impl<'r, 'a, I> Drop for PutBackOnDrop<'r, 'a, I> {
+            fn drop(&mut self) {
+                self.into.inner.set(self.inner.take())
+            }
+        }
+        let pbod = PutBackOnDrop {
+            inner: self.inner.take(),
+            into: self,
+        };
+        Self {
+            inner: Cell::new(pbod.inner.clone()),
+            sep: self.sep,
+        }
+    }
+}


Test patch: