Reputation: 65
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;
}
Upvotes: 2
Views: 118
Reputation: 51910
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.
Upvotes: 1
Reputation: 385325
Your delete is here:
StringT() {
delete [] m_str;
}
But that's a constructor, not a destructor. 😉
Upvotes: 5