mattreecebentley / plf_list

A drop-in replacement for std::list with 293% faster insertion, 57% faster erasure, 17% faster iteration and 77% faster sorting on average. 20-24% speed increase in use-case testing.
https://plflib.org/list.htm
zlib License
151 stars 21 forks source link

Move constructor crashed #11

Closed tsung-wei-huang closed 5 years ago

tsung-wei-huang commented 5 years ago

Hi I suspected the move semantics are not handled properly. We are using your project but it crashed when constructing a list with move constructor.

        list(list &&source) PLF_LIST_NOEXCEPT:
            element_allocator_type(source),
            groups(std::move(source.groups)),
            end_node(std::move(source.end_node)),
            last_endpoint(std::move(source.last_endpoint)),
            end_iterator(reinterpret_cast<node_pointer_type>(&end_node)),
            begin_iterator((source.begin_iterator.node_pointer == source.end_iterator.node_pointer) ? reinterpret_cast<node_pointer_type>(&end_node) : std::move(source.begin_iterator)),
            node_pointer_allocator_pair(source.node_pointer_allocator_pair.total_number_of_elements),
            node_allocator_pair(source.node_allocator_pair.number_of_erased_nodes)
        {
            end_node.previous->next = begin_iterator.node_pointer->previous = end_iterator.node_pointer;
            source.groups.blank();
            source.node_pointer_allocator_pair.total_number_of_elements = 0;

                        source.reset();   // NEED to clean out source to avoid crash
        }
clin99 commented 5 years ago

Here is an example:

#include <iostream>                                                                                                                    
#include "plf_list.h"

int main() {
  plf::list<int> list1;
  list1.emplace_back(1);
  list1.emplace_back(2);

  plf::list<int> list2 = std::move(list1);

  list1.emplace_back(3);  // Reuse the moved list will cause segmentation fault
}

My GCC version is 7.4.0 and the compilation command I use:

g++ -std=c++17 example.cpp

Thanks!

mattreecebentley commented 5 years ago

Alright, will fix

mattreecebentley commented 5 years ago

Fixed- and have included that test in the test suite - thanks. The move assignment operator was doing the right thing, just not the move constructor.

BTW your assumptions about what you can do with a moved container can be incorrect depending on circumstance - the standard requires that destruction works on a moved type but that's it. However most containers tend to follow the core guidelines and make the container clear()'d, and the STL does this, so I should be doing it too! More info if interested: https://stackoverflow.com/questions/9168823/reusing-a-moved-container http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-move-semantic