rlogiacco / CircularBuffer

Arduino circular buffer library
GNU Lesser General Public License v3.0
312 stars 85 forks source link

Passing elements of non-fundamental type by reference #53

Closed jpasqua closed 2 years ago

jpasqua commented 3 years ago

First of all, thank you for this library. It has been very useful.

I have a fork which allows the user (or the compiler) to choose whether to pass elements to push() and unshift() by value or reference. The choice is controlled by a #define:

/**
 * For the push() and unshift() functions, you may choose whether to pass-by-value, pass-by-reference,
 * or have the template choose at compile time based on the element type. This is the default and in this case
 * we pass-by-value for fundamental types and pass-by-reference otherwise. To always use pass-by-value, which
 * is what the original library did, #define ARG_TYPE_VAL *before* including CircularBuffer.h. To always
 * use pass-by-reference #define ARG_TYPE_REF. Don't define either to get the default behavior.
 */
#include <type_traits>
template<typename T>
#if defined(ARG_TYPE_VAL)
    using choose_arg_type = T;
#elif defined(ARG_TYPE_REF)
    using choose_arg_type = const T &;
#else
    using choose_arg_type = typename std::conditional<std::is_fundamental<T>::value, T,  const T&>::type;
#endif

The functions then look like this:

/**
 * Adds an element to the beginning of buffer: the operation returns `false` if the addition caused overwriting an existing element.
 */
bool unshift(choose_arg_type<T> value);

/**
 * Adds an element to the end of buffer: the operation returns `false` if the addition caused overwriting an existing element.
 */
bool push(choose_arg_type<T> value);

I have also added a peekAt():

    /**
     * Similar to operator [], but returns a const ref to the stored element rather than a copy.
     * Similar to std::vector::at, but always returns a const reference. 
     * Calling this operation using and index value greater than `size - 1` returns the tail element.
     * *WARNING* Calling this operation on an empty buffer has an unpredictable behaviour.
     */
    const T& peekAt(IT index) const;

Not very useful for fundamental types, but can be useful for larger objects.

I'm happy to pass along the source or create a pull request if you're interested.

rlogiacco commented 3 years ago

I see a problem in inlcuding your changes: you are introducing a dependency on std library, which is not always available and also increases the library footprint, which is a problem with microcontrollers with very limited memory, like the Attiny or the ATMega328. Can you tell how much is the footprint growing?

If I get your point, you are still happy to store the elements by value using their pointers, but you wish to allow the write operations to work differently, depending on your necessity.

In my opinione this would be best achieved by decorating or extending the CircularBuffer rather than modifying it. If you contribute your feature in the form of a derived class or a decorating wrapper, users will not have to pay for the additional program space used unless they need to....

What do you think about this?

jpasqua commented 2 years ago

I apologize for the late reply - somehow I missed the notification. Good point about the dependency on std, though I believe using std::conditional and std::is_fundamental are compile-time only so should not increase the footprint. I will check.

Your suggestion is probably best. For other reasons I have since created a simplified Circular Buffer class that meets my needs a little better. I'll close this issue to get it off your radar.

Again, thanks for all the work you put into this.