Destructor deleting memory premature

Question

I am quiet new to memory management in C++. I made a BigInt class that is now fully implemented except for the destructor which is impacting performance of the program. However, when I try to implement the destructor my program crashes.

In the following code for multiplying BigInts:

BigInt& BigInt::operator*=(BigInt const& other) {

    //copy of this and other
    BigInt* tempThis = new BigInt(*this); //1st number
    BigInt* tempOther = new BigInt(other); //2nd number

    //create temps so we can use value of BigInt before it is changed
    BigInt* sum = new BigInt(0); //holds the eventual answer

    BigInt* i = new BigInt(0);

    //add *this BigInt to sum otherTemp amount of times
    //this will yield multiplication answer.
    for (*i; *i < *tempOther; *i = *i + 1) {
        *sum += *this;
    }

    *this = *sum;

    return *this;

}

The destructor is called when *i = *i + 1 is called in the for loop and then I think it gets deleted in my destructor which looks like this:

// destructor
BigInt::~BigInt() {
    delete[] this->bigIntVector;
}

// copy constructor
BigInt::BigInt(BigInt const& orig)
    : isPositive(orig.isPositive)
    , base(orig.base)
{
    this->bigIntVector = new BigIntVector(*(orig.bigIntVector));
}

Once 'i' is deleted nothing works and the whole program breaks.

If someone could give me a few pointers about destructors and how to fix my problem it would be great help. Thanks.

C++ is not Java. That function is full of memory leaks. Why are you using `new` in so many places (and not a single call to `delete`)? Why not use the copy constructor (which you should have written) to create temporary BigInt's? — PaulMcKenzie, Nov 14 '15 at 20:42
I thought I was using the copy constructor but I guess not. Do you have a suggestion for reformatting with less memory use? — sanic, Nov 14 '15 at 20:43
For example, this: `BigInt* tempThis = new BigInt(*this); //1st number` Should be this: `BigInt tempThis = *this;` and also this: `BigInt sum(0);` instead of what you have now. If this doesn't work correctly, then you need to take a step back and implement the copy constructor correctly (and in addition, the assignment operator). — PaulMcKenzie, Nov 14 '15 at 20:45
Also, your loop to multiply is flawed, unless you implemented an operator `+` for `BigInt` that takes an `int` argument. The reason why it's flawed is that you won't be able to handle the case where you are multiplying by a BigInt that is greater than the max value of a regular `int` or `long` type. — PaulMcKenzie, Nov 14 '15 at 20:49
I have implemented the neccessary '+' for that. If I change my code according to your suggestion will the destructor also work better or is that a seperate issue? — sanic, Nov 14 '15 at 20:50
The destructor is fine. There is nothing wrong with it. What *is* wrong is that you didn't implement a correct copy constructor and assignment operator. In other words the "rule of 3" needs to be adhered to. — PaulMcKenzie, Nov 14 '15 at 20:51
I added my copy constructor, can you take a look and make sure I did that correctly? — sanic, Nov 14 '15 at 20:53
whoops, I added the wrong one, I updated it with actual BigInt copy constructor — sanic, Nov 14 '15 at 20:55
Where is the assignment operator? http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — PaulMcKenzie, Nov 14 '15 at 20:56
No, it does not cover it. If you want proof: `{ BigInteger b(10); BigInteger b2(20); b = b2;}` Try that, you will see double deletion errors and memory leaks when that `{ }` block is exited. — PaulMcKenzie, Nov 14 '15 at 20:57
wow okay maybe I got my work cut out for me then. Thank you. — sanic, Nov 14 '15 at 20:58
It wouldn't hurt to learn the programming language you're trying to program in. — juanchopanza, Nov 14 '15 at 21:20

Walter · Answer 1 · 2015-11-14T21:35:54.417

In C++, the same (horrible) arithmetic could be implemented as follows.

BigInt& BigInt::operator*=(BigInt const& other)
{
  if(other==0)
    return other;
  if(other> 1)
    for(BigInt old=*this,i=1; i!=other; ++i)
      operator+=old;
  else if(other<0) {
    BigInt old=*this;
    *this=0;
    for(BigInt i=0; i!=other; --i)
      operator-=old;
  }
  return*this;
}

assuming that the constructor from int, the copy constructor, the increment operator++ and the addition operator+= are all properly implemented (as well as the destructor).

Unfortunately, you failed to give us more information, but your copy constructor and destructor are definitely broken:

this->bigIntVector = new BigIntVector(*(orig.bigIntVector));

is followed by

delete[] this->bigIntVector;

giving you undefined behaviour (allocating with new but deallocating with delete[] -- delete[] is for memory allocated with new[]). I suspect you meant to copy the memory from the original in the copy constructor. However, you don't. If

class BigInt {
  size_t    size=0;                // number of some_types allocated
  some_type*bigIntVector=nullptr;  // ptr to memory allocated, if any
  /* rest of class */
};

then the copy constructor could be implemented like (assuming size is non-static)

BigInt::BigInt(BigInt const&orig)
: size(orig.size()                                   // copy size
, bigIntVector(size? new some_type[size] : nullptr)  // allocate memory
{ std::memcpy(orig.bigIntVector, bigIntVector); }    // copy memory

However, (almost) the same could be implemented much easier with

class BigInt
{
  std::vector<some_type> bigIntVector;
public:
  BigInt(BigInt const&) = default;
  BigInt(BigInt &&) = default;
  BigInt&operator=(BigInt const&) = default;
  BigInt&operator=(BigInt &&) = default;
  / * rest of class */
};

when the copy and move constructors (as well as the respective assignment operators) are automatically correctly created for you. You only need to address the default constructor, for example

BigInt::BigInt()                    // default constructor
: bigIntVector(1,some_type(0)) {}   // size=1, value=0

and the constructors from built-in integer types. If you're new to C++, avoid new and delete in favour of standard library containers.

And once you stop being new to C++, avoid 99% or more of the places where you think of using `new` and `delete`, typically in favour of standard containers ... — Christopher Creutzig, Nov 14 '15 at 21:50

Destructor deleting memory premature

1 Answers1