search

alexanderBaranov / OOP

OOP
0 stars 0 forks source link

Замечания по CMyString #18

Closed alexey-malov closed 8 years ago

alexey-malov commented 8 years ago 
CMyString::CMyString() throw()
{
    m_size = 0;
    m_chars = make_unique<char[]>('\0');
}

данный конструктор может выбросить исключение (потому как make_unique использует new). если функция, помечена как noexcept или throw() выбросит исключение вызовется terminate

make_unique<char[]>('\0'); выделяет блок размером 0 байт (+служебные данные) в куче и возвращает ненулевой указатель. Фактически разыменовывать этот указатель нельзя. Если вызвать GetStringData() и передать его хоть в printf, то будет неопределенное поведение из-за разыменования этого указателя

В любом случае - это невалидное состояние строки

alexey-malov commented 8 years ago 
CMyString::CMyString(const char * pString, unsigned length)
{
    assert(pString);

    m_size = length;

    m_chars.reset(new char[m_size + 1]);
    memset(m_chars.get(), '\0', m_size + 1);
    memcpy(m_chars.get(), pString, m_size);
}

зачем дважды заполнять массив, когда можно скопировать m_size байт и в конце добавить '\0'

alexey-malov commented 8 years ago 
CMyString const CMyString::SubString(unsigned start, unsigned length /*= UINT_MAX*/) const
{
    if (start + length > m_size)
    {
        return *this;
    }

    return CMyString(m_chars.get() + start, length);
}

на 64 битной платформе будет разная разрядность типов unsigned и size_t. Символы строки свыше 4Гб будут недоступны для этой функции используй size_t для start и length

CMyString("Hello").SubString(3, 5)

вернет "Hello", что не вяжется с ожиданиями

Сделай в тестах разбор возмоных ситуаций. за эталон бери STL

alexey-malov commented 8 years ago 
void CMyString::Clear()
{
    m_chars.reset(nullptr);
}

После этого GetStringData() будет возвращать нулевой указатель, что приведет к неопределенному поведению при использовании. Кроме того, не будут выполнен инвариант "длина строки равна нулю"

alexey-malov commented 8 years ago

Для каждого метода класс String выпиши следующее: -предусловия (что ожидаем от аргументов) -постусловия (возвразащемый результат и ожидаемое состояние объекта) вызов каждого метода должен переводить объект из одного валидного состояния в другое. методы, бросающие исключение должны предоставлять строгую гарантию безопасности исключений. В худшем случае - базовую.

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago 
CMyString::CMyString(const char * pString, unsigned length)
{
    assert(pString);

    m_size = length;

    m_chars.reset(new char[m_size + 1]);
    memcpy(m_chars.get(), pString, m_size);
    m_chars.get()[m_size] = '\0';
}

как на 64-битной платформе передать строку длиннее 4Гб?

alexey-malov commented 8 years ago

Тесты неструктурированы. Особенно тесты конструктора. Там даже проверяется Substr

alexey-malov commented 8 years ago 
    {
        CMyString str("cat");
        const char *data = str.GetStringData();
        BOOST_CHECK_EQUAL(data, "cat");
        BOOST_CHECK_EQUAL(data[str.GetLength()], '\0'); // предыдущий Check интерпретирует строку как zero-terminated, и не прошел бы в случае отсутствия символа с кодом 0
    }
    {
        CMyString str("catdog", 3);
        const char *data = str.GetStringData();
        BOOST_CHECK_EQUAL(data, "cat");
        BOOST_CHECK_EQUAL(data[str.GetLength()], '\0');
    }
        CMyString str("cat", 3);
        const char *data = str.GetStringData();
        BOOST_CHECK_EQUAL(data, "cat");
        BOOST_CHECK_EQUAL(data[str.GetLength()], '\0');
    }
``` аналогично
alexey-malov commented 8 years ago 
    {
        CMyString str("cat", 10);
        const char *data = str.GetStringData();
        BOOST_CHECK_EQUAL(data, "cat");
        BOOST_CHECK_EQUAL(data[str.GetLength()], '\0');
    }

этот тест может привести к неопределенному поведению (выход за пределы массива). Нужно сделать так:

        CMyString str("cat\0dog\0pie", 10);

содержимое сравнить через memcmp с "cat\0dog\0pi\0"

alexey-malov commented 8 years ago 
        CMyString str;
        BOOST_CHECK_EQUAL(str.GetStringData(), "");

длину проверить.

Желательно запилить вспомогательную функцию для сравнения.

template <size_t N>
void ExpectZeroTerminatedStringData(CMyString const& s, const char (&data)[N])
{
    static_assert(N > 0, "Non-zero array is expected");
    BOOST_REQUIRE_EQUAL(s[N - 1], '\0');
    BOOST_REQUIRE_EQUAL(s.GetLength(), N - 1);
    BOOST_REQUIRE_EQUAL(memcmp(s.GetStringData(), data, N), 0u);
    BOOST_REQUIRE_EQUAL(s.GetStringData()[s.GetLength()], '\0');
}

Вместо

CMyString str("cat");
const char *data = str.GetStringData();
BOOST_CHECK_EQUAL(data, "cat");
BOOST_CHECK_EQUAL(data[str.GetLength()], '\0');

достаточно написать

ExpectZeroTerminatedStringData(CMyString("cat"), "cat");

существующие проверки заменить на подобную функцию

alexey-malov commented 8 years ago 
CMyString const CMyString::SubString(size_t start, size_t length /*= UINT_MAX*/) const
{
    if (start >= m_size)
    {
        return CMyString();
    }

    if (start + length > m_size)
    {
        return CMyString(m_chars.get() + start, m_size - start);
    }

    return CMyString(m_chars.get() + start, length);
}

лучше возвращать неконстантный CMyString. Это позволит использовать перемещающий конструктор или оператор присваивания.

BOOST_AUTO_TEST_CASE(testSubstring)
{
    CMyString str("Hello");
    CMyString fer = str.SubString(0, 6);

    BOOST_CHECK_EQUAL(fer.GetStringData(), "Hello");
    BOOST_CHECK_EQUAL(str.SubString(3, 5).GetStringData(), "lo");
    BOOST_CHECK_EQUAL(str.SubString(10, 5).GetStringData(), "");
}

использовать ExpectZeroTerminatedStringData, а также проверить граничное условие (start=string.length).

alexey-malov commented 8 years ago 
CMyString::CMyString(const char * pString, unsigned length)
{
    assert(pString);

    m_size = length;

    m_chars.reset(new char[m_size + 1]);
    memcpy(m_chars.get(), pString, m_size);
    m_chars.get()[m_size] = '\0';
}

В тестах рассмотреть граничные случаи: -переданная длина = 0 -в конце строки находится символ с кодом 0

alexey-malov commented 8 years ago 
    if ((index > m_size) || (index < 0))
    {
        throw exception("string subscript out of range");
    }

в STL есть разные классы исключений. Здесь больше подходит out_of_range Проверка index < 0 всегда false и не имеет смысла. Тип size_t - беззнаковый

BOOST_AUTO_TEST_CASE(testIndexedAccess)
{
    CMyString str;
    BOOST_CHECK(str[0] == '\0');
    BOOST_CHECK_THROW(str[1], std::exception);

    CMyString hello("Hello");
    BOOST_CHECK(hello[0] == 'H');
    BOOST_CHECK_EQUAL(hello[5], '\0');
    BOOST_CHECK_THROW(hello[6], std::exception);
}

Этот тест проверяет работу лишь неконстантного оператора [].

alexey-malov commented 8 years ago 
CMyString& CMyString::operator+=(const CMyString &other)
{
    if (!other.GetLength())
    {
        return *this;
    }

    CMyString tmp = *this + other;
    *this = tmp;

    return *this;
}

В результате *this + other конструируется временный объект Он перемещается в tmp потом копируется в this Лучше перемещать в this сразу, минуя временный объект

alexey-malov commented 8 years ago 
CMyString& CMyString::operator=(const CMyString &other)
{
    if (this == &other)
    {
        return *this;
    }

    m_size = other.GetLength();
    m_chars.reset(new char [m_size + 1]);
    memcpy(m_chars.get(), other.GetStringData(), m_size + 1);

    return *this;
}

если new бросить исключение, то текущий объект будет в невалидном состоянии (m_size хранит новую длину, а указатель m_chars будет указывать на старую строку).

alexey-malov commented 8 years ago 
int CompairStrings(const CMyString &leftString, const CMyString &rightString)

сравнить переводится как compare

alexey-malov commented 8 years ago 
int CompairStrings(const CMyString &leftString, const CMyString &rightString)
{
    size_t sizeOfLeftString = leftString.GetLength();
    size_t sizeOfRightString = rightString.GetLength();

    size_t cmpSize = (sizeOfLeftString > sizeOfRightString) ? sizeOfLeftString : sizeOfRightString;
    return memcmp(leftString.GetStringData(), rightString.GetStringData(), cmpSize);
}

Ты сравниваешь блоки памяти длиной с бОльшую строку, что приводит к выходу за пределы области памяти строки меньшей длины и к неопределенному поведению

alexey-malov commented 8 years ago 
CMyString operator +(const CMyString &leftString, const CMyString &rightString)
{
    if (leftString.Empty() && rightString.Empty() )
    {
        return "";
    }

    size_t sizeOfLeftString = leftString.GetLength();
    size_t sizeOfRightString = rightString.GetLength();

    size_t length = leftString.GetLength() + rightString.GetLength() + 1;
    std::unique_ptr<char[]> chars(new char[length]);

    memcpy(chars.get(), leftString.GetStringData(), sizeOfLeftString);
    memcpy(chars.get() + sizeOfLeftString, rightString.GetStringData(), sizeOfRightString);

    chars.get()[length - 1] = '\0';

    return CMyString(chars.get(), length-1);
}

Здесь происходит создание временного массива, копирование в него данных, из которого потом снова данные копируются в CMyString. Избавиться от лишнего копирования.

alexey-malov commented 8 years ago 
CMyString::CMyString(CMyString && other)
{
    *this = move(other);
}

При отсутствии перемещающего оператора присваивания такой перемещающий конструктор выполняет копирование, а не перемещение.

Автоматически компилятор не сгенерирует в твоем случае оператор перемещения

http://en.cppreference.com/w/cpp/language/move_assignment

alexey-malov commented 8 years ago

поскольку в классе есть нетривиальные операторы перемещающего и копирующего присваивания, их тоже следует покрыть тестами. Как и перемещающий конструктор и копирующий конструкторы

alexey-malov commented 8 years ago 
BOOST_AUTO_TEST_CASE(after_moving_the_object_must_be_in_a_valid_state)
{
    CMyString donor("Hello");
    CMyString acceptor("world");
    // Operator = must return the reference to the left argument
    BOOST_CHECK_EQUAL(&(acceptor = move(donor)), &acceptor);
    ExpectZeroTerminatedStringData(acceptor, "Hello");
    ExpectZeroTerminatedStringData(donor, "");

    // Donor must be in a valid state
    donor += "new content";
    ExpectZeroTerminatedStringData(donor, "new content");

}
1>------ Build started: Project: TestCMyString, Configuration: Debug Win32 ------
1>  TestCMyString.cpp
1>  TestCMyString.vcxproj -> F:\teaching\ips\2014\students\baranov\OOP\lab6\CMyString\Debug\TestCMyString.exe
1>  Running 14 test cases...
1>  unknown location(0): fatal error in "after_moving_the_object_must_be_in_a_valid_state": memory access violation
1>  f:\teaching\ips\2014\students\baranov\oop\lab6\cmystring\testcmystring\testcmystring.cpp(17): last checkpoint
1>  
1>  *** 1 failure detected in test suite "TestsOfCMyString"
alexey-malov commented 8 years ago

Каждый метод/оператор должен переводить объект из одного валидного состояния в то же самое, либо другое валидное состояние. Из всех точках выхода, включая выход по исключению.

Каждый используемый методом объект (передаваемый через аргументы метода, или на который у объекта есть ссылка или указатель (агрегация/композиция), либо глобальный объект) должен либо остаться в том же состоянии, либо перейти в новое валидное состояние.

Валидное состояние - состояние, в котором данные объекта находятся в согласованном состоянии.

Это должно проверяться в тестах

alexey-malov commented 8 years ago 
CMyString SubString(size_t start, size_t length = UINT_MAX)const;

На 64-битной платформе UINT_MAX остается 32-битным. Используй SIZE_MAX

alexey-malov commented 8 years ago 
CMyString operator +(const CMyString &leftString, const CMyString &rightString)
{
    if (leftString.Empty() && rightString.Empty())
    {
        return "";
    }

    size_t sizeOfLeftString = leftString.GetLength();
    size_t sizeOfRightString = rightString.GetLength();

    size_t length = sizeOfLeftString + sizeOfRightString;

    CMyString str(leftString.GetStringData(), length);
    memcpy((char*)str.GetStringData(), leftString.GetStringData(), sizeOfLeftString);
    memcpy((char*)str.GetStringData() + sizeOfLeftString, rightString.GetStringData(), sizeOfRightString);

    return str;
}

Преобразование типов в стиле C в прграммах на C++ использовать не следует. Снятие константности с переменной - тоже признак кода с тухлятиной.

CMyString(unique_ptr<char[]> && data, size_t length);

Вариант с += считаю предпочтительнее. если пользоваться этим конструктором, то вызывающая сторона должна не забыть выделить память на 1 символ больше и разместить в конце символ с кодом 0. Дружба без крайней необходимости не нужна

alexey-malov commented 8 years ago 
CMyString::CMyString(CMyString && other)
{
    *this = move(other);

    other.m_chars.reset(new char[1]);
    other.m_chars.get()[0] = '\0';
}

new может выбросить исключение. В итоге объект-донор останется в состоянии, не позволяющем его дальнейшее использование (нулевой указатель на данные и невалидная длина)

alexey-malov commented 8 years ago 
CMyString& CMyString::operator+=(const CMyString &other)
{
    if (other.Empty())
    {
        return *this;
    }

    auto chars = move(m_chars);

    size_t length = m_size + other.GetLength();
    m_chars.reset(new char[length + 1]);

    memcpy(m_chars.get(), chars.get(), m_size);
    memcpy(m_chars.get() + m_size, other.GetStringData(), other.GetLength() + 1);

    m_size = length;

    return *this;
}

Если new char[length + 1] выбросит исключение, объект останется в невалидном состоянии (нулевой указатель на данные)

alexey-malov commented 8 years ago 
BOOST_AUTO_TEST_CASE(MalovsTest)
{
    CMyString donor("Hello");
    CMyString acceptor(donor);
    CMyString result = "1234";
    result += acceptor;
    ExpectZeroTerminatedStringData(result, "Hello1234");
}

Для перемещающего конструктора не был написан тест. В итоге, он работает некорректно.

alexey-malov commented 8 years ago 
CMyString& CMyString::operator=(CMyString &&other)
{
    if (this == &other)
    {
        return *this;
    }

    m_chars = move(other.m_chars);
    m_size = other.m_size;

    other.m_size = 0;
    other.m_chars.reset(new char[1]);
    other.m_chars.get()[0] = '\0';

    return *this;
}

Если оператор new в other.m_chars.reset(new char[1]);, то объект-донор останется в невалидном состоянии (нулевой указатель на данные).

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago 
CMyString& CMyString::operator=(CMyString &&other)
{
    if (this == &other)
    {
        return *this;
    }

    m_chars = move(other.m_chars);
    m_size = other.m_size;

    other.m_chars = make_unique<char[]>(1);
    other.m_chars.get()[0] = '\0';
    other.m_size = 0;

    return *this;
}

если new внутри make_unique() выбросит исключение, то other.m_pchars останется равным nullptr и прежней длиной. Попытка использовать его (использовать указатель GetStringData(), который должен возвращать указатель на zero-terminated char array), вернет нулевой указатель, что приведет.

Должны выполняться инварианты как в объекте-доноре, так и в объекте акцепторе:

alexey-malov commented 8 years ago 
CMyString::CMyString(CMyString && other)
{
    *this = move(other);

    other.m_chars = make_unique<char[]>(1);
    other.m_chars.get()[0] = '\0';
    other.m_size = 0;
}

Конструкторе копирования используется код, повторяющий операции перемещающего оператора присваивания. Если оператор присваивания справился со своей задачей, то нет смысла повторно это делать. Если оператор присваивания не справился с задачей, то до ее выполнения в конструкторе копирования дело не дойдет, т.к. управление будет передано в обработчик снаружи

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago 
CMyString& CMyString::operator=(CMyString &&other)
{
    if (this == &other)
    {
        return *this;
    }

    CMyString temp;

    m_chars = move(other.m_chars);
    m_size = other.m_size;

    other.m_size = 0;
    other.m_chars = move(temp.m_chars);

    return *this;
}

использование temp здесь излишне, тем более, что при его объявлении может выброситься исключение.

Исправлено на:

CMyString& CMyString::operator=(CMyString &&other)
{
    if (this == &other)
    {
        return *this;
    }

    m_chars = move(other.m_chars);
    m_size = other.m_size;

    other.m_size = 0;

    return *this;
}
alexey-malov commented 8 years ago 
CMyString::CMyString()
{
    SetEmptyString();
}

Если вместо SetEmptyString перевести строку в "пустое" состояние (как при перемещении), то этот конструктор не будет требовать никаких выделений памяти и не будет выбрасывать исключений вообще.

Не исправлено

alexey-malov commented 8 years ago 
const char* CMyString::GetStringData() const
{
    return m_size ? m_chars.get() : "";
}

Проверять корректнее не размер, а указатель m_chars на неравенство nullptr

Исправлено на:

char* CMyString::GetStringDataImpl() const
{
    return (m_chars != nullptr) ? m_chars.get() : "";
}

const char* CMyString::GetStringData() const
{
    return GetStringDataImpl();
}
alexey-malov commented 8 years ago 
void CMyString::Clear()
{
    if (m_size)
    {
        SetEmptyString();
    }
}

Если отказаться от SetEmptyString, то можно реализовать noexcept Clear

Исправлено

alexey-malov commented 8 years ago 
CMyString& CMyString::operator+=(const CMyString &other)
{
    if (other.Empty())
    {
        return *this;
    }

    auto chars = move(m_chars); // забрали из m_chars данные (m_chars теперь nullptr)

    size_t length = m_size + other.GetLength();
    m_chars = make_unique<char[]>(length + 1); // !!!может выбросить исключение

    memcpy(m_chars.get(), chars.get(), m_size);
    memcpy(m_chars.get() + m_size, other.GetStringData(), other.GetLength() + 1);

    m_size = length;

    return *this;
}

Если make_unique выбросит исключение, то текущая строка останется в невалидном состоянии (nullptr-указатель на данные и ненулевая длина). Пользоваться строкой будет нельзя. В реализации свести к необходимому минимуму количество выделений памяти и копирования символов:

  1. Выделить память под хранение результирующей строки
  2. Скопировать в нее данные левой и правой строки
  3. Заменить данные строки и ее длину на новые

Заменено на:

CMyString& CMyString::operator+=(const CMyString &other)
{
    if (other.Empty())
    {
        return *this;
    }

    size_t length = m_size + other.GetLength();
    auto chars = make_unique<char[]>(length + 1);

    memcpy(chars.get(), GetStringData(), m_size);
    memcpy(chars.get() + m_size, other.GetStringData(), other.GetLength() + 1);

    m_chars = move(chars);
    m_size = length;

    return *this;
}
alexey-malov commented 8 years ago 
bool CompareStrings(const CMyString &leftString, const CMyString &rightString)
{
    size_t sizeOfLeftString = leftString.GetLength();
    size_t sizeOfRightString = rightString.GetLength();

    return (sizeOfLeftString == sizeOfRightString) ?
        memcmp(leftString.GetStringData(), rightString.GetStringData(), sizeOfRightString) == 0 :
        false;
}

Название функции выбрано неудачно. Уместее будет StringsAreEqual

Исправлено

alexey-malov commented 8 years ago 
CMyString operator +(const CMyString &leftString, const CMyString &rightString)
{
    if (leftString.Empty() && rightString.Empty())
    {
        return "";
    }

    CMyString str(leftString); // Создается копия левой строки
    str += rightString;          // Создается итоговая строка, в которую копируется копия левой и правая.

    return str;
}

Желательно реализовать без лишнего выделения: (выделить под результат, скопировать левую и правую половины). Предлагаемый вариант - приватный конструктор, принимающий unique_ptr (по rvalue-ссылке) и длину. Оператор + сделать другом

Заменено на

CMyString operator +(const CMyString &leftString, const CMyString &rightString)
{
    if (leftString.Empty() && rightString.Empty())
    {
        return "";
    }

    auto temp = std::make_unique<char[]>(leftString.m_size + rightString.m_size + 1);
    memcpy(temp.get(), leftString.GetStringData(), leftString.m_size);
    memcpy(temp.get() + leftString.m_size, rightString.GetStringData(), rightString.m_size + 1);

    return CMyString(std::move(temp), leftString.m_size + rightString.m_size);
}
alexey-malov commented 8 years ago

При отключении SDL-проверок валятся тесты 2016-07-22 10_00_35-cmystring - microsoft visual studio administrator SDL добавляет доп. код, который, в частности, заполняет память, содержащуюся в объекте, нулями

BOOST_AUTO_TEST_CASE(testIndexedAccess)
{
    CMyString str;
    BOOST_CHECK(str[0] == '\0');
    BOOST_CHECK_THROW(str[1], std::out_of_range);

    CMyString hello("Hello");
    BOOST_CHECK(hello[0] == 'H');
    BOOST_CHECK_EQUAL(hello[5], '\0');
    BOOST_CHECK_THROW(hello[6], std::out_of_range);
}
alexey-malov commented 8 years ago 
size_t CMyString::GetLength() const
{
    return m_chars ? m_size : 0;
}

Это "костыль", маскирующий нарушение инвариантов объекта. Поле m_size следует держать валидным