Valgrind found 3 memory leaks but I can't figure out where they are

Question

I tried to implement the basic String class by myself and it's working fine, but Valgrind says there are 3 memory leaks and I can't figure out where and why. I really tried to delete everything after it isn't used anymore (I have started using Valgrind today). Now I'm really concerned about my basic C/C++ memory management knowledge. I made a comment to the places in the code where Valgrind found a leak (//VALGRIND). I've also uploaded a screenshot of this error message click to see the screenshot.

EDIT: I have updated the screenshot, so you can see the full output.

StringT.h

template<typename char_type = char>
class StringT {
public:

 explicit StringT(const char_type *str) {
        if (str != nullptr) {
            size_t len = strlen(str);
            m_str = new char_type[len + 1]; //VALGRIND: 6 bytes in 1 blocks are definitely lost in loss record 1 of 3
            strcpy(m_str, str);
        }
    }

   ~StringT() {
        delete [] m_str;
    }


StringT(const StringT & other) {
        size_t len = 0;
        if (other.m_str) len = strlen(other.m_str);
        m_str = new char_type[len + 1]; //VALGRIND: 6 bytes in 1 blocks are definitely lost in loss record 2 of 3
        strcpy(m_str, other.m_str);
    }

    StringT(StringT && other) noexcept {
        m_str = other.m_str;
        other.m_str = nullptr;
    }


     StringT & operator+=(const StringT &other) {
        if (other.m_str == nullptr) //when other str is empty just return current Str
            return *this;

        const size_t mysize{m_str ? strlen(m_str) : 0}; // check if not null then call strlen
        const size_t osize{other.m_str ? strlen(other.m_str) : 0};

        char *newStr = new char_type[osize + mysize + 1]; //VALGRIND: 11 bytes in 1 blocks are definitely lost in loss record 3 of 3
        newStr[0] = '\0'; //strcat searches for '\0', so newStr has to be a valid String

        if (m_str) strcat(newStr, m_str);
        if (other.m_str) strcat(newStr, other.m_str);

        delete[] m_str; //delete old string
        m_str = newStr; //set member to new concatenated str

        return *this;
    }

    size_t length() const {
        if (!m_str) return 0;
        return strlen(m_str);
    }


    friend
    std::ostream &operator<<(std::ostream &out, StringT<> &other) {
        if (other.m_str) out << other.m_str;
        return out;
    }

private:
    char_type *m_str{nullptr};
};

main.cpp

int main() {

    const char *cArr = "Hello";
    const char *cArr2 = "World";
    StringT<char> hello(cArr);
    StringT<char> world(cArr2);
    StringT<char> emptyStr;

    std::cout << "hello: " << hello << std::endl;
    std::cout << "world: " << world << std::endl;
    std::cout << "emptyStr: " << emptyStr << std::endl;

    StringT<char> hCopy(hello);
    StringT<char> wMove(std::move(world));

    std::cout << "hCopy: " << hello << std::endl;
    std::cout << "hCopy: " << hCopy << std::endl;
    std::cout << "world: " << world << std::endl;
    std::cout<<  "wMove: " << wMove << std::endl;
    std::cout<<  "lenMove: " << wMove.length() << std::endl;
    std::cout<<  "lenEmptyStr: " << emptyStr.length() << std::endl;

    hello += wMove;
    std::cout<<  "hello += world: " << hello << std::endl;

    return 0;
}

Answer 1

As already answered, the delete needs to be in the destructor. However, the proper solution to your problem is to not do memory management manually in this case. You should be using std::unique_ptr for your m_str member:

std::unique_ptr<char_type[]> m_str;

This frees you from having to new and delete manually. This also helps protects against memory leaks in case of exceptions. Even if you delete everything you allocated, you can still have memory leaks if an exception occurs between the new and the delete. unique_ptr helps in preventing this kind of leak.

Your class only needs small changes:

template<typename char_type = char>
class StringT {
public:
    StringT()
    {}

    explicit StringT(const char_type *str)
    {
        if (str != nullptr) {
            size_t len = strlen(str);
            m_str = std::make_unique<char_type[]>(len + 1);
            strcpy(m_str.get(), str);
        }
    }

    StringT(const StringT & other)
    {
        size_t len = 0;
        if (other.m_str)
            len = strlen(other.m_str.get());
        m_str = std::make_unique<char_type[]>(len + 1);
        strcpy(m_str.get(), other.m_str.get());
    }

    StringT(StringT && other) noexcept
    {
        m_str = std::move(other.m_str);
    }

    StringT & operator+=(const StringT &other)
    {
        if (other.m_str == nullptr)
            return *this;

        const size_t mysize{m_str ? strlen(m_str.get()) : 0};
        const size_t osize{strlen(other.m_str.get())};

        auto newStr = std::make_unique<char_type[]>(osize + mysize + 1);
        newStr[0] = '\0';

        if (m_str)
            strcat(newStr.get(), m_str.get());
        strcat(newStr.get(), other.m_str.get());

        m_str = std::move(newStr);
        return *this;
    }

    size_t length() const
    {
        if (!m_str)
            return 0;
        return strlen(m_str.get());
    }

    friend
    std::ostream &operator<<(std::ostream &out, StringT<> &other)
    {
        if (other.m_str)
            out << other.m_str.get();
        return out;
    }

private:
    std::unique_ptr<char_type[]> m_str;
};

You will notice that there are no calls to new or delete in this code. m_str will automatically delete allocated memory on its own when needed.

Answer 2

Your delete is here:

StringT() {
    delete [] m_str;
}

But that's a constructor, not a destructor. 😉