Замечания по CMyStack

[alexey-malov]

2>  test.cpp
2>c:\teaching\2016\new\lelekov\oop\lab7\cmystack\test\test.cpp(102): warning C4267: 'argument': conversion from 'size_t' to 'const int', possible loss of data
2>c:\teaching\2016\new\lelekov\oop\lab7\cmystack\test\test.cpp(121): warning C4267: 'argument': conversion from 'size_t' to 'const int', possible loss of data
2>  main.cpp

• [x] Программа должна компилироваться без предупреждений компилятора во всех конфигурациях и платформах.

[alexey-malov]

        std::shared_ptr<Node> newElement(new Node);

• [x] используйте make_shared. Подробности в презентации "Современный подход к разработке C++-приложений"

[alexey-malov]

    void Copy(std::shared_ptr<Node> lastElement)
    {
        if (lastElement != nullptr)
        {
            Copy(lastElement->prevElement);
            Push(lastElement->element);
        }
    }

    void Move(std::shared_ptr<Node> lastElement)
    {
        if (lastElement != nullptr)
        {
            Move(lastElement->prevElement);

            auto copy = std::move(lastElement->element);
            Push(copy);
        }
    }

• [x] Со стеком в несколько десятков тысяч элементов закончится стек и программа упадет. Решить задачу копирования стека без использования рекурсии за O(N) по времени и O(1) дополнительной памяти.

• [x] Задачу перемещения содержимого одного стека в другой решить за O(1) по времени и O(1) доп. памяти

[alexey-malov]

        BOOST_AUTO_TEST_CASE(this_test_will_probably_fail)
        {
            CMyStack<std::string> s1;
            s1.Push("kjhlskjfhsdkljfhslkjfhskljfhs dlfkjhsdlfkjshf 1");
            s1.Push("kjhlskjfhsdkljfhslkjfhskljfhs dlfkjhsdlfkjshf 1");
            s1.Push("kjhlskjfhsdkljfhslkjfhskljfhs dlfkjhsdlfkjshf 1");
            s1.Push("kjhlskjfhsdkljfhslkjfhskljfhs dlfkjhsdlfkjshf 1");
            s1 = s1;
            BOOST_CHECK_EQUAL(s1.GetSize(), 4);
            s1.Pop();
            s1.Pop();
            s1.Pop();
            s1.Pop();
            BOOST_CHECK(s1.IsEmpty());
        }

• [x] Проблема с копирующим и перемещающим самоприсваиванием. Покрыть тестами, исправить

[alexey-malov]

    void Pop()
    {
        std::shared_ptr<Node> tmpNode = m_lastElement;

        if (IsEmpty())
        {
            throw std::domain_error("Can't pop element from empty stack.");
        }

        m_lastElement = m_lastElement->prevElement;

        tmpNode.reset();

        --m_size;
    }

• [x] Много лишнего кода. Весь код, касающийся tmpNode - бесполезный

[alexey-malov]

    int GetSize()const
    {
        return(m_size);
    }

• [x] Для хранения размера использовать size_t

[alexey-malov]

    void Copy(CMyStack const &stack)
    {
        Clear();
        auto tmpNode = stack.m_lastElement;

        auto seed = std::make_shared<Node>();
        auto prevNode = seed;

        seed->element = tmpNode->element;

        tmpNode = tmpNode->prevElement;

        while (tmpNode != nullptr)
        {
            auto newNode = std::make_shared<Node>();
            newNode->element = tmpNode->element;

            prevNode->prevElement = newNode;
            prevNode = newNode;

            tmpNode = tmpNode->prevElement;
        }

        m_size = stack.GetSize();
        m_lastElement = seed;
    }

• [x] Метод Copy, вызываемый оператором копирующего присваивания, не обеспечивает строгую гарантию безопасности исключений, т.к. предварительно очищает список. Легче реализовать оператор копирующего присваивания через создание временной копии и перемещение (либо обмен) в текущий объект.

[alexey-malov]

    struct Node
    {
        T element = T();
        std::shared_ptr<Node> prevElement = nullptr;
    };

• [x] Лучше назвать иначе. element -> data, prevElement - prevNode

[alexey-malov]

    void Move(CMyStack &stack)
    {
        if (*this != stack)
        {
            m_lastElement = stack.m_lastElement;
            m_size = stack.GetSize();

            stack.m_lastElement = nullptr;
            stack.m_size = 0;
        }
    }

• [x] Аргумент следует принимать по rvalue-ссылке.

[alexey-malov]

• [ ] Для копирующего и перемещающего операторов присваивания должны быть свои тесты

        BOOST_AUTO_TEST_CASE(can_be_moved_via_assignment_operator)
        {
            CMyStack<int> newIntStack = (std::move(intStack));
        }

Тут у вас вызов перемещающего коснтруктора, а не оператора перемещающего присваивания

• [ ] Для копирующего оператора присваивания замечание остается прежним

[alexey-malov]

    void Copy(CMyStack const &stack)
    {
        auto tmpNode = stack.m_lastElement;

        auto seed = std::make_shared<Node>();
        auto prevNode = seed;

        seed->data = tmpNode->data;

        tmpNode = tmpNode->prevNode;

        while (tmpNode != nullptr)
        {
            auto newNode = std::make_shared<Node>();
            newNode->data = tmpNode->data;

            prevNode->prevNode = newNode;
            prevNode = newNode;

            tmpNode = tmpNode->prevNode;
        }

        m_size = stack.GetSize();
        m_lastElement = seed;
    }

• [ ] Если во время работы Copy выбросится исключение, то разрушение цепочки узлов стека произойдет рекурсивно. Надо перехватывать исключение и в обработчике итеративно удалять элементы цепочки