Issue or Design Decision?

[melvyniandrag]

The dumpBuf_byVal method logs the wrong values. Did you deliberately omit copy and assignment in the code because people probably won't use it, or do you think it's inappropriate to have these methods?

#include <CircularBuffer.h>

const int N = 5;
CircularBuffer<float, N> buf;

void dumpBuf_byVal( CircularBuffer<float, N> buf ){
  for ( int i = 0; i < buf.size(); i++ ){
    Serial.print(buf[i]);
    Serial.print(" ");
  }
  Serial.println();
  Serial.flush();
}

void dumpBuf_byRef( CircularBuffer<float, N> &buf ){
  for ( int i = 0; i < buf.size(); i++ ){
    Serial.print(buf[i]);
    Serial.print(" ");
  }
  Serial.println();
  Serial.flush();
}

void setup() {
  Serial.begin(9600);
  float f= 1.0;
  for( int i = 0; i < N; ++i ){
    buf.push(f);
  }
  dumpBuf_byVal( buf );
  dumpBuf_byRef( buf );
}

void loop() {
  // put your main code here, to run repeatedly:
}

[rlogiacco]

Would you expect to have a shallow or a deep copy?

[Erlkoenig90]

A shallow copy would hardly be possible reasonably, since the memory is allocated inside the CircularBuffer class. Therefore we need to do a deep copy, which also is convention with C++ classes & containers. We have to re-set the head/tail pointers properly, however, since they would point to the original elements otherwise. Alternatively we can use indices instead of pointers. Similar to #16, we should also make sure that the copy constructors / assignment operators of elements are only called if they exist. I could add that to the PR...

[rlogiacco]

Considering this library aims at microcontrollers and memory is a big limitation I would discourage or even disable the whole pass-by-value rather than supporting it via a deep copy. I can already imagine a user invoking a function and ending up with a hardware rest due to memory corruption...

I could understand a shallow copy, but that would have other implications in terms of unexpected side effects.

[rlogiacco]

I'm willing to solve this by disabling the copy constructor and the assignment operator based on the principle a deep copy is too memory expensive:

private: 
    CircularBuffer(const CircularBuffer&); // no implementation 
    CircularBuffer& operator=(const CircularBuffer&); // no implementation 

[Erlkoenig90]

A more modern way would be:

    CircularBuffer(const CircularBuffer&) = delete;
    CircularBuffer(CircularBuffer&&) = delete;
    CircularBuffer& operator=(const CircularBuffer&) = delete;
    CircularBuffer& operator=(CircularBuffer&&) = delete;

The visibility shoud probably be public: as that probably results in better compiler errors. I added the rvalue variants for completeness.

[rlogiacco]

A more modern way would be:

This makes me feel damn old 👴