Fixing valgrind notifications #1078
Conversation
| @@ -378,7 +378,6 @@ namespace xt | |||
| inline auto xbuffer_owner_storage<CP, A>::operator=(self_type&& rhs) -> self_type& | |||
| { | |||
| swap(rhs); | |||
| rhs.m_moved_from = true; | |||
There was a problem hiding this comment.
this was a memory leak because the swapped to rhs didn't deallocate.
| @@ -347,6 +347,7 @@ namespace xt | |||
| row_major_result<> rm; | |||
| dyn_opt_ass_type vec; | |||
| vec.resize(rm.m_shape, layout_type::row_major); | |||
| vec.fill(123); | |||
There was a problem hiding this comment.
this stuff made valgrind complain about conditional jumps depending on uninitialized memory.
| using owner_adaptor = xbuffer_adaptor<double*&, acquire_ownership>; | ||
|
|
||
| TEST(xbuffer_adaptor, owner_destructor) | ||
| { | ||
| size_t size = 100; | ||
| double* data = new double[size]; | ||
| double* data = allocator{}.allocate(size); |
There was a problem hiding this comment.
weird that we were not deleting in the tests.
Why using an allocator btw?
There was a problem hiding this comment.
we're not deleting because that's what the owning buffer adaptor is supposed to do and the allocator stuff because of otherwise mismathcing new / delete
| @@ -482,7 +482,7 @@ namespace xt | |||
| // Allocate memory | |||
| m_word_size = std::size_t(atoi(&typestring[2])); | |||
| m_n_bytes = compute_size(shape) * m_word_size; | |||
| m_buffer = new char[m_n_bytes]; | |||
| m_buffer = std::allocator<char>{}.allocate(m_n_bytes); | |||
There was a problem hiding this comment.
What change does it make to use the allocator instead of the new operator?
Another question is: If we use std::allocator::allocate, shouldn't we use std::allocator::deallocate instead of delete on the other side?
There was a problem hiding this comment.
that's what's used in the buffer adaptor
|
All makes sense. Merging when all is green. |
With these changes valgrind stops complaining.
The big ones: not using realloc in svector anymore. This might hit performance somewhat but I think it's better to have a clean valgrind output.
Use the std::allocator to allocate memory for the owning buffer. That one is a bit annoying but I think warranted for library functions. Otherwise you get an error because of a mismatch between
freeandnew ... []where valgrind wants one to use the correspondingdelete[]instead offreeor whatever the allocator is using.The workaround would be to write a shim allocator that calls dealloc with
delete[]There was also some tiny bugs in the tests as well as one memory leak with move assignment on owning buffers.