ЗАмечания по stack

[alexey-malov]

    void Push(T const& data)
    {
        try
        {
            Node* newNode = new Node; //  (1)
            newNode->data = data; // (2)
            newNode->next = m_pTop;
            m_pTop = newNode;
        }
        catch (...)
        {
            throw;
        }
    }

• [x] Если (2) выбросит исключение, то память, выделенная в (1) утечёт

Добавьте в Node конструктор, принимающий строку и указатель на следующий элемент. Тогда если вызвать:

Node* newNode = new Node(data, m_pTop);

Если выбросится исключение во время работы конструктора Node (инициализация поля data), то область памяти под Node будет зачищена в операторе new.

[alexey-malov]

    void Pop()
    {
        if (!IsEmpty())
        {
            m_pTop = m_pTop->next; // (1)
        }
        else
        {
            throw std::logic_error("Stack is empty");
        }
    }

• [x] Т.к. m_pTop указатель "сырой", то при его присваивании не произойдёт автоматического освобождения памяти, на которую он раньше указывал. Надо вызывать delete

[alexey-malov]

    void Clear()
    {
        if (!IsEmpty())
        {
            while (m_pTop)
            {
                Node* currNode = m_pTop;
                m_pTop = currNode->next;
                delete currNode;
            }
        }
        else
        {
            throw std::logic_error("Stack is empty");
        }
    }

• [x] Очистка пустого стека не должна выбрасывать исключений

[alexey-malov]

• [x] У класса должен быть деструктор, вызывающий Clear

[alexey-malov]

    T GetTop()
    {
        if (!IsEmpty())
        {
            return m_pTop->data;
        }
        else
        {
            throw std::logic_error("Stack is empty");
        }
    }

• [x] Метод должен быть константным

[alexey-malov]

• [x] Конструктор копирования сделать за один проход

[alexey-malov]

• [x] Оператор копирующего присваивания можно реализовать через copy and swap

[alexey-malov]

    struct Node
    {
        Node(T data, Node* next)
            : data(data)
            , next(next)
        {
        }
        T data;
        Node* next;
    };

• [x] тут лучше data(std::move(data)) сделать

[alexey-malov]

Код копирования можно сократить до:

            Node* srcCurrentNode = other.m_pTop;
            Node** dstCurrentNode = &m_pTop;
            while (srcCurrentNode)
            {
                *dstCurrentNode = new Node(srcCurrentNode->data, nullptr);
                dstCurrentNode = &(*dstCurrentNode)->next;
                srcCurrentNode = srcCurrentNode->next;
            }

[alexey-malov]

    CMyStack<T> operator=(CMyStack const& other)
    {
        if (this != &other)
        {
            if (m_pTop)
            {
                Clear(); // (1)
            }
            *this = CMyStack(other); // (2)
        }
        return *this;
    }

• [x] Если (2) выбросит исключение, то из-за (1) вы останетесь с пустым стеком

[alexey-malov]

    CMyStack<T> operator=(CMyStack&& other)
    {
        std::swap(*this, &other);
        return *this;
    }

• [ ] Тут лучше сделать Clear и забрать указатель себе. А также добавить защиту от самоперемещения

[alexey-malov]

    void Push(T const& data)
    {
        try
        {
            Node* newNode = new Node(data, m_pTop);
            m_pTop = newNode;
        }
        catch (...)
        {
            throw;
        }
    }

• [x] Ловить и сразу перевыбрасывать исключения не имеет смысла

[alexey-malov]

    void Pop()
    {
        if (!IsEmpty())
        {
            Node* tmp = m_pTop;
            try
            {
                m_pTop = m_pTop->next; (1)
            }
            catch (...)
            {
                delete tmp; (2)
            }
        }
        else
        {
            throw std::logic_error("Stack is empty");
        }
    }

• [x] Тут утечка памяти: (1) не бросает исключений, следовательно (2) никогда не выполнится

[alexey-malov]

    T GetTop() const
    {
        if (!IsEmpty())
        {
            return m_pTop->data;
        }
        else
        {
            throw std::logic_error("Stack is empty");
        }
    }

• [x] Тут лучше возвращать константную ссылку, чтобы не создавать копию

[alexey-malov]

    CMyStack<T>& operator=(CMyStack&& other)
    {
        if (this != &other)
        {
            Clear();
            this = &other;
        }
        return *this;
    }

• [ ] Этот код не скомпилируется. this - константный указатель

[alexey-malov]

• [x] Оператор копирующего присваивания тестами не покрыт

            WHEN("using operator =")
            {
                CMyStack<int> other = stack;
                THEN("the elements of the stacks are equal")

• [x] Здесь вызовается не оператор присваивания, а конструктор копирования.

[alexey-malov]

Идеальная передача и универсальные ссылки в C++

https://habr.com/ru/post/242639/

[alexey-malov]

• [x] Добавьте noexcept к сигнатурам методов, конструкторов и операций класса CMyStack, гарантирующих отсутствие исключений

[alexey-malov]

    CMyStack<T>& operator=(CMyStack&& other) noexcept
    {
        if (this != &other)
        {
            Clear();
            this->m_pTop = other.m_pTop; // (1)
        }
        return *this;
    } 

• [ ] Оператор не протестирован. В нём ошибка - в (1) оба стека будут указывать на одну и ту же структуру данных, что приведёт к UB при разрушении второго стека, использующего указатель на уже удалённые данные памяти