-1

I build a simple string class. I try to make the concatenation functions which one of them is + and the other +=. When trying to implement += I generate a Str object under that equals the 1st string and which size is s.size(). But then when I tried to append to it a new string t I need to free the old array s string and allocate new size for it. After the destructor of the temp Str object is called it stucks there when freeing the old space and I can't understand why. How can I deallocate the Str under the + member function?

class Str
{

public:
    typedef size_t size_type;
    typedef char* iterator;
    typedef const char* const_iterator;
    iterator begin(){ return p; }
    iterator end() { return p + std::strlen(p); }
    const_iterator begin() const { return p; }
    const_iterator end() const { return p + std::strlen(p); }

    size_type size() const { return data_length; }
    Str() {};


    Str(const Str& s): 
        p(new char[s.size() +1]), 
        data_length(s.size())
    {
        std::copy(s.begin(), s.end(), p);
        p[data_length] = '\0';
    }

    Str(const char* cp) :
        p(new char[std::strlen(cp) + 1 ]), 
        data_length(std::strlen(cp))
    {
        std::copy(cp, cp+ std::strlen(cp) + 1,p);//copies also the '\0' char to the last place in p
    }


Str& operator=(Str& rhs)//assignment operator
{
    if (&rhs != this)
    {
        uncreate();
        create(rhs.size());
        std::copy(rhs.begin(), rhs.end() + 1, p);
        //p[rhs.size()] = '\0';
    }
    return *this;
}

Str& operator=(const char* cp)//assignment operator
{
    if (cp!= p)
    {
        uncreate();
        create(std::strlen(cp));
        std::copy(cp, cp+std::strlen(cp), p);
        p[data_length] = '\0';
    }
    return *this;
}




    Str& operator+=(const Str&);

    ~Str() 
    {
        delete[] p;//stucked here while returning from + member function
        data_length = 0;
    }

    const char* c_str() const;
    void copy(char* ,size_type);
private:
    char* p;
    size_type data_length = 0;

    const_iterator ci() const { return p; }

    void uncreate();
    void create(size_type);
};



Str operator+(const Str& s, const Str& t)
{
    Str r = s;
    r += t;
    return r;   

}


inline Str& Str::operator+=(const Str &s)
{
    //trying to allocate new space for this object 

    std::copy(s.begin(),s.end(),p+this->size());
    p[data_length] = '\0';
    return *this;
}

void Str::create(Str::size_type n)
{
    p = new char[n + 1];
    data_length = n;
}

void Str::uncreate()
{
    delete[] p;//to check that p is allocated right
    data_length = 0;
}

The main for example:

int main()
{

    Str s1 = "hello"; 
    Str s2 = "worly";
    Str s3 = s1 + s2;

    return 0;
}
axcelenator
  • 1,497
  • 3
  • 18
  • 42
  • 2
    You just [don't](https://stackoverflow.com/questions/46991224/are-there-any-valid-use-cases-to-use-new-and-delete-raw-pointers-or-c-style-arr)! Use `std::string` instread. – user0042 Jan 01 '18 at 13:55
  • I try to dealocate the `r` object? can I do that? – axcelenator Jan 01 '18 at 13:57
  • What is wrong with doing it in the destructor like you already are? – super Jan 01 '18 at 14:04
  • Show your code for Str::operator+= . All else you have in your question appears ok at first glance. – Nitesh Jan 01 '18 at 14:06
  • @Nitesh hello, now shown as previously – axcelenator Jan 01 '18 at 14:08
  • You just have a comment saying _trying to allocate_, but no code actually allocating anything. Your existing buffer at `p` is pretty much guaranteed not to be big enough for `this->size() + s.size()` characters, which is what you need. – Useless Jan 01 '18 at 14:11
  • @Useless hello, that is why I started this question. How can I allocate enough memory for `this` and `s` ? – axcelenator Jan 01 '18 at 14:13
  • @axcelenator as expected, the operator += does not allocate more memory for the characters being added. You need to allocate new memory to p for new size. – Nitesh Jan 01 '18 at 14:13
  • @axcelenator but in your question the problem you ask is about deallocation: "How can I deallocate the Str under the + member function?" – Nitesh Jan 01 '18 at 14:14
  • @Nitesh because the `+` function calls the `+=` I don't know what is the wright place for the deallocation. Do I need just to do `delete[] p`? – axcelenator Jan 01 '18 at 14:17
  • 2
    If this is for production work, use std::string. If this is an educational exercise, you should have exactly *one* call to strlen in your code - in the constructor from `const char*`, to initialize `datalen`. Every other call should use `datalen` instead. (That way, your string - like std::string - can contain embedded nul characters.) – Martin Bonner supports Monica Jan 01 '18 at 14:18
  • 1
    @axcelenator : You don't need a deallocator. The destructor of `r` in operator + will do the deallocation. Of course, in `operator +=`, you need to 1. allocate enough memory. 2. copy the initial portion from the old memory. 3. copy the appended string from the rhs. 4. deallocate the old memory. 5. store the pointer in `this->p` (step 3 needs to come before step 4, in case someone does `s+=s`) – Martin Bonner supports Monica Jan 01 '18 at 14:21

1 Answers1

2

I suppose you want something like this:

inline Str& Str::operator+=(const Str &s)
{
    const int new_data_length = data_length + s.data_length;
    char * temp = new char[new_data_length + 1];
    memcpy(temp, p, data_length);
    memcpy(temp + data_length, s.p, s.data_length);

    delete [] p;
    p = temp;

    data_length = new_data_length;
    p[data_length] = 0;

    return *this;
}
p-a-o-l-o
  • 9,807
  • 2
  • 22
  • 35
  • @p-a-o-l-o hello when returning from `+` operator the delete[] operator in the destructor stopps the program – axcelenator Jan 01 '18 at 14:40
  • You *must not* delete the old `p` until after you copied from `s.p` - they may be the same variable (`s += s;`) – Martin Bonner supports Monica Jan 01 '18 at 14:41
  • @MartinBonner - Good catch, hadn't thought of that when I commented to replace p with temp, although I think the concept still applies, copy everything into `temp` and then once all the copying is done, delete `p` and then assign `temp` to `p`. The extra copy back step is unnecessary. – pstrjds Jan 01 '18 at 14:44
  • You cannot `delete [] temp` after assigning it to `p`. – pstrjds Jan 01 '18 at 14:50
  • Thanks, commenters. Maybe, I shouldn't be posting at all, today :) – p-a-o-l-o Jan 01 '18 at 14:51