ShiqiYu / CPP

Lecture notes, projects and other materials for Course 'CS205 C/C++ Program Design' at Southern University of Science and Technology.
2.07k stars 351 forks source link

MyString's copy assignment operator can't handle self-assignment correctly and not exception safe #30

Open ltimaginea opened 2 years ago

ltimaginea commented 2 years ago

https://github.com/ShiqiYu/CPP/blob/8d450448b8006f82fed677deb0d3639bf2226915/week11/examples/example2/mystring.hpp#L29-L48

Issue1: can't handle self-assignment

See: Assignment Operators, C++ FAQ (isocpp.org) If x = x , bad errors will occur. We can handle self-assignment by explicitly testing for self-assignment:

MyString& operator=(const MyString& ms)
{
    if (this != &ms)
    {
        create(ms.buf_len, ms.characters);
    }
    return *this;
}

Issue2: not exception safe

In create function, if new char[this->buf_len]{}; throws an exception, *this won't keep a valid state. So, copy assignment operator is not exception safe. The solution is copy the underlying data firstly, then delete *this's old resource:

bool create(int buf_len, const char* data)
{
    char* new_characters = NULL;
    if (buf_len != 0)
    {
        new_characters = new char[buf_len] {};
    }
    if ((new_characters != NULL) && (data != NULL))
        strncpy(new_characters, data, buf_len);

    release();

    this->buf_len = buf_len;
    this->characters = new_characters;

    return true;
}

See also

prathameshatkare commented 5 days ago

The second issue is related to exception safety. In the create() function, if the memory allocation for new char[buf_len] throws an exception, the original state of the object (*this) may already be altered, leading to an invalid or inconsistent state (like leaked memory). This breaks the "basic exception safety guarantee," which requires that an object remain in a valid state if an exception is thrown.

Solution: Copy-and-Swap Idiom for Exception Safety A safer way to handle both issues (self-assignment and exception safety) is to use the copy-and-swap idiom. This pattern involves copying the data first, ensuring that memory allocation succeeds before modifying the original object.

Here’s how the MyString class can be improved:

Improved MyString Class cpp Copy code

include // for strncpy

include // for std::swap

class MyString { private: char* characters; int buf_len;

public: // Constructor MyString(int buf_len = 0, const char* data = nullptr) : buf_len(buf_len), characters(nullptr) { if (buf_len != 0) { characters = new char[buf_len]{}; if (data != nullptr) { strncpy(characters, data, buf_len); } } }

// Copy Constructor
MyString(const MyString& ms)
    : buf_len(ms.buf_len), characters(nullptr)
{
    if (buf_len != 0) {
        characters = new char[buf_len]{};
        strncpy(characters, ms.characters, buf_len);
    }
}

// Destructor
~MyString() {
    release();
}

// Assignment Operator (Copy-and-Swap Idiom)
MyString& operator=(MyString ms) {
    swap(*this, ms);  // Swap *this with the temporary copy
    return *this;
}

// Helper function to release memory
void release() {
    delete[] characters;
    characters = nullptr;
    buf_len = 0;
}

// Swap function to exchange resources between two objects
friend void swap(MyString& first, MyString& second) noexcept {
    using std::swap;
    swap(first.buf_len, second.buf_len);
    swap(first.characters, second.characters);
}

};

prathameshatkare commented 5 days ago

Issue 1: Self-Assignment In C++, the assignment operator needs to handle self-assignment. Without a self-assignment check, code like x = x can lead to logical errors or data corruption, particularly if resources like dynamic memory are managed. In the MyString example, the assignment operator is correctly checking for self-assignment using the line if (this != &ms) to ensure that the assignment operation only proceeds if the source object (ms) is not the same as the target object (*this).