From d1e235cc9fa2b6a2469150426c0a3e32892283cb Mon Sep 17 00:00:00 2001
From: Remo Senekowitsch <remo@buenzli.dev>
Date: Sun, 10 Sep 2023 17:29:24 +0200
Subject: [PATCH] Improve example solution of circular-buffer

The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
https://github.com/exercism/rust/pull/1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
---
 .../practice/circular-buffer/.meta/example.rs | 50 +++++++++----------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/exercises/practice/circular-buffer/.meta/example.rs b/exercises/practice/circular-buffer/.meta/example.rs
index 489891063..b85bcb558 100644
--- a/exercises/practice/circular-buffer/.meta/example.rs
+++ b/exercises/practice/circular-buffer/.meta/example.rs
@@ -4,20 +4,21 @@ pub enum Error {
     FullBuffer,
 }
 
-pub struct CircularBuffer<T: Default + Clone> {
-    buffer: Vec<T>,
-    size: usize,
+pub struct CircularBuffer<T> {
+    /// Using Option leads to less efficient memory layout, but
+    /// it allows us to avoid using `unsafe` to handle uninitialized
+    /// mempory ourselves.
+    data: Vec<Option<T>>,
     start: usize,
     end: usize,
 }
 
-impl<T: Default + Clone> CircularBuffer<T> {
-    // this circular buffer keeps an unallocated slot between the start and the end
-    // when the buffer is full.
-    pub fn new(size: usize) -> CircularBuffer<T> {
-        CircularBuffer {
-            buffer: vec![T::default(); size + 1],
-            size: size + 1,
+impl<T> CircularBuffer<T> {
+    pub fn new(capacity: usize) -> Self {
+        let mut data = Vec::with_capacity(capacity);
+        data.resize_with(capacity, || None);
+        Self {
+            data,
             start: 0,
             end: 0,
         }
@@ -27,8 +28,9 @@ impl<T: Default + Clone> CircularBuffer<T> {
         if self.is_empty() {
             return Err(Error::EmptyBuffer);
         }
-
-        let v = self.buffer[self.start].clone();
+        let v = self.data[self.start]
+            .take()
+            .expect("should not read 'uninitialized' memory");
         self.advance_start();
         Ok(v)
     }
@@ -37,18 +39,16 @@ impl<T: Default + Clone> CircularBuffer<T> {
         if self.is_full() {
             return Err(Error::FullBuffer);
         }
-
-        self.buffer[self.end] = byte;
+        self.data[self.end] = Some(byte);
         self.advance_end();
         Ok(())
     }
 
     pub fn overwrite(&mut self, byte: T) {
-        if self.is_full() {
+        self.data[self.end] = Some(byte);
+        if self.start == self.end {
             self.advance_start();
         }
-
-        self.buffer[self.end] = byte;
         self.advance_end();
     }
 
@@ -57,24 +57,22 @@ impl<T: Default + Clone> CircularBuffer<T> {
         self.end = 0;
 
         // Clear any values in the buffer
-        for element in self.buffer.iter_mut() {
-            std::mem::take(element);
-        }
+        self.data.fill_with(|| None);
     }
 
-    pub fn is_empty(&self) -> bool {
-        self.start == self.end
+    fn is_empty(&self) -> bool {
+        self.start == self.end && self.data[self.start].is_none()
     }
 
-    pub fn is_full(&self) -> bool {
-        (self.end + 1) % self.size == self.start
+    fn is_full(&self) -> bool {
+        self.start == self.end && self.data[self.start].is_some()
     }
 
     fn advance_start(&mut self) {
-        self.start = (self.start + 1) % self.size;
+        self.start = (self.start + 1) % self.data.len();
     }
 
     fn advance_end(&mut self) {
-        self.end = (self.end + 1) % self.size;
+        self.end = (self.end + 1) % self.data.len();
     }
 }