jnk0le / Ring-Buffer

simple C++11 ring buffer implementation, allocated and evaluated at compile time
MIT License
380 stars 64 forks source link

add support for non trivial objects #23

Open jnk0le opened 1 year ago

jnk0le commented 1 year ago

should use move mechanic or put another static assert

markisus commented 10 months ago

Can you elaborate on this? Looking through the code it's not obvious what or where a failure would occur.

jnk0le commented 10 months ago

Currently insert/removal uses copy constructors everywhere, (i.e. T a = b;) which als means memory leak if T is allocating anything.

markisus commented 10 months ago

As I understand, this should work fine if the author of the class has properly implemented the copy assignment operator. For example, if T is std::shared_ptr, the copy assign would first decrement the usage count of the pointer held in a and then increment the usage count of the pointer held in b.

jnk0le commented 10 months ago

copy assignment

ah, assignment, not constructor.

It will still leak until given slot is reused for new data, not to mention that the buffer array is uninitialized.

I'll add the assert until figuring out the std::move + rval refs and all of the related "what ifs".

markisus commented 10 months ago

I think the slot would hold onto memory until reuse, but that’s much better than a leak because the buffer size is bounded.

Anyway thanks for the library!

huweiATgithub commented 6 months ago

The copy assignment a = b is ok if the copy assignment operator of type T is implemented. It is responsibility of the maintainer of type T to ensure that current resources is freed before copy resources from others.

Is my understand correct?

jnk0le commented 5 months ago

Currently the buffer array is uninitialized, so that will be very undefined operation.

This would also make the copy constructors unusable for it's intended purpose.

huweiATgithub commented 5 months ago

Currently the buffer array is uninitialized, so that will be very undefined operation.

This would also make the copy constructors unusable for it's intended purpose.

I see. So, if the type can be default constructed, then it will be fine?

jnk0le commented 5 months ago

As long as all members are also trivially (default) constructible.