0

I have a class that contains the operator + () to add 2 objects. The class contains an attribute char * variable str that points to an allocated char array from new char[] that contains a C style text string. I want the operator +() to concatenate two char arrays into a third char array allocated as a new buffer sized to contain the concatenated string as the result of the operator.

I am using overloaded constructor and initializing list and I initialize the variable to a default value. The destructor I've added to the class frees the memory allocated for the object text buffer in the char *str variable.

Everything works perfectly fine WITHOUT the class destructor, but as soon as I add the destructor, the program prints weird characters. I am also seeing a memory leak.

Here is the code and note that the class destructor is included here!

#include <iostream>
#include <cstring>
#include<cstring>

class Mystring {
private:
    char *str;      
public:
    Mystring():
        str{nullptr} {
        str = new char[1];
        *str = '\0';
        }
    Mystring(const char *s):
         str {nullptr} {
        if (s==nullptr) {
            str = new char[1];
            *str = '\0';
        } else {
            str = new char[std::strlen(s)+1];
            std::strcpy(str, s);
        }
    }
    Mystring(const Mystring &source):
        str{nullptr} {
        str = new char[std::strlen(source.str)+ 1];
        std::strcpy(str, source.str);
        std::cout << "Copy constructor used" << std::endl;
    }
    ~Mystring(){
          delete [] str;
    }

    void print() const{
          std::cout << str << " : " << get_length() << std::endl;
    }

    int get_length() const{
        return std::strlen(str); 
    }
    const char *get_str() const{
        return str;
    }
    Mystring&operator+(const Mystring &dstr)const;

};

// Concatenation
Mystring &Mystring::operator+(const Mystring &dstr)const {
    char *buff = new char[std::strlen(str) + std::strlen(dstr.str) + 1];
    std::strcpy(buff, str);
    std::strcat(buff, dstr.str);
    Mystring temp {buff};
    delete [] buff;
    return temp;
}

int main() {

    Mystring m{"Kevin "};
    Mystring m2{"is a stooge."};
    Mystring m3;
    m3=m+m2;
    m3.print();

    return 0;
}
Richard Chambers
  • 14,509
  • 3
  • 62
  • 86
Finixzz
  • 21
  • 7

2 Answers2

3

Mystring &Mystring::operator+(const Mystring &dstr)const

This states that it's going to return a reference to a MyString Your temp however is local. This results in a far more serious issue than just a memory leak.

Once you're returning by value; you'll find that it might end up wanting to use the copy or move constructor; which leads to (as said in the comments) the rule of 3 (or rule of 5).

UKMonkey
  • 6,473
  • 3
  • 17
  • 29
1

This

Mystring &Mystring::operator+(const Mystring &dstr)const {
    char *buff = new char[std::strlen(str) + std::strlen(dstr.str) + 1];
    std::strcpy(buff, str);
    std::strcat(buff, dstr.str);
    Mystring temp {buff};
    delete [] buff;
    return temp;
}

contains a bug. You create a temp object and return it by reference (Mystring&). That reference turns into a dangling one as soon as the function returns and using it is undefined behaviour. The fact that the code appears to work with or without the destructors means nothing. Undefined behaviour is undefined and you cannot expect anything from the code that invokes it.

You may simply change the return type of Mystring::operator+ to return by value (simply Mystring).

Although, you would encounter another problem later on. 

m3=m+m2;

performs a copy assignment, which uses T& operator = (const T&). You did not provide one, so the compiler generated one.

Why is that bad? Because automatically generated special member functions perform shallow copies (if they are used for copying), which means that if you have, for example, pointer member variable, the pointer will be copied. Just the pointer. Not the data it points to. Just the pointer. You end up with two pointers pointing to the same memory and once a correctly defined destructor tries to free those resources, you encounter a double delete[] error, which, again, is undefined behaviour for non-nullptr pointers.

You may want to look forward into the rule of three, and then probably into the rule of five.

Fureeish
  • 9,869
  • 3
  • 23
  • 46
  • So, if I'm wrong correct me. I need to return object by value and overload (=) . Is this legit ? Mystring &Mystring::operator=(Mystring &&dstr){ if(this==&dstr){ return *this; } delete [] str; str=dstr.str; dstr.str=nullptr; return *this; } – Finixzz Jun 27 '19 at 15:56
  • @Finixzz That is only the move assignment operator. It would be sufficient in this very particular case, but to follow the rule of three you need the copy assignment operator `Mystring& operator=(const Mystring& dstr)`. Adding `Mystring& operator=(Mystring&& )` and `Mystring(Mystring&&)` would then also satisfy the rule of five. – Max Langhof Jun 27 '19 at 15:58
  • @Finixzz why do you insist on implementing *move-assignment* operator, when I specifically mentioned *copy-assignment* operator? In your example, yes, this is "*legit*", but if you provide move operators, provide the copy ones too. Additionally, you might be interested in [copy and swap idiom](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). – Fureeish Jun 27 '19 at 15:58
  • @Fureeish Because it is clear, that it is more sufficient than copy in this particular case – Finixzz Jun 27 '19 at 18:18
  • @Finixzz but is the any *confusion* about this? What's the point of this comment? The fact that in your particular case, move assignment operator is used does not mean that you should implement only that one. Again - what's your question/concern now? – Fureeish Jun 27 '19 at 18:29
  • @Fureeish My only concern now is why are you concerned about my suggest of implementing move assignment in this particular case. Chill of, and thanks for feedback ! – Finixzz Jun 27 '19 at 18:36