Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for non trivial objects #23

Open
jnk0le opened this issue Jul 16, 2023 · 9 comments
Open

add support for non trivial objects #23

jnk0le opened this issue Jul 16, 2023 · 9 comments

Comments

@jnk0le
Copy link
Owner

jnk0le commented Jul 16, 2023

should use move mechanic or put another static assert

@markisus
Copy link

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

@jnk0le
Copy link
Owner Author

jnk0le commented Jan 15, 2024

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

@markisus
Copy link

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
Copy link
Owner Author

jnk0le commented Jan 16, 2024

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".

@jnk0le jnk0le changed the title non trivial copy constructors will break add support for non POD data Jan 16, 2024
@markisus
Copy link

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!

@jnk0le jnk0le closed this as completed in 900d531 Jan 17, 2024
@jnk0le jnk0le changed the title add support for non POD data add support for non trivial objects Jan 17, 2024
@jnk0le jnk0le reopened this Jan 17, 2024
@huweiATgithub
Copy link

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
Copy link
Owner Author

jnk0le commented May 23, 2024

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
Copy link

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
Copy link
Owner Author

jnk0le commented May 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants