assignment operator in class, _ASSERTE(_BLOCK_TYPE_IS_VALID(pHead->nBlockUse));

Question

I currently have an assignment operator that looks like this:

CellPhoneHandler CellPhoneHandler:: operator=(const CellPhoneHandler &original){

   if (this->nrOfCellphones > 0)
        for (int i = 0; i < this->nrOfCellphones; i++) {
             delete this->cellphone[i];
   }


    delete[] cellphone;

    this->nrOfCellphones = original.nrOfCellphones;
    this->cellphone = new CellPhone*[this->nrOfCellphones];


    for (int i = 0; i<this->nrOfCellphones; i++) 
    {
        cellphone[i] = original.cellphone[i];
    }

    return *this;
}

and then in the start of the program, I intend to test that it is working:

    CellPhoneHandler assignme;
assignme.addPhone("assignment phone", 500, 1000);
assignme.addPhone("assignment phone 2", 500, 1000);

copyme = assignme;

Hower, when i exit the program i get and unhandled exception which points to line 52 in dbgdel.cpp:

       /* verify block type */
        _ASSERTE(_BLOCK_TYPE_IS_VALID(pHead->nBlockUse));

Any ideas on why? The problem seems to lie only within this functions, as it works when i comment the test away from the program.

my CellPhone class looks like this:

    class CellPhone
{
private:
    string model;
    int stock;
    double price;
public:
    CellPhone(string model="", int stock=0, double price=0); // constructor
    string getModel()const;
    int getStock()const;
    double getPrice()const;
    void setModel(string model);
    void setStock(int stock);
    void setPrice(double price);
    string toString() const;
    ~CellPhone(); //destructor
};

Changed the variable name to original, but the error is still the same.

class CellPhoneHandler
{

private:
    CellPhone **cellphone;
    int nrOfCellphones;

public:
    CellPhoneHandler();
    CellPhoneHandler(const CellPhoneHandler &original); 
    CellPhoneHandler operator=(const CellPhoneHandler &original); 
    void addPhone(string model, int price, int stock);
    ~CellPhoneHandler();
    string showByStock(int stock) const;
    void removeModel(string model);
    void changePriceProcent(double procent, int price);
    void showAll(string array[], int nrOfCellphones) const;
    void saveToFile(string fileName) const;
    void readFromFile(string fileName);
    int getNrOfPhones()const;
};

UPDATE: changed my operator= to this, simplified code:

CellPhoneHandler CellPhoneHandler:: operator=(const CellPhoneHandler &original){
CellPhoneHandler tmp(original);
swap(this->cellphone, tmp.cellphone);
swap(this-> nrOfCellphones, tmp.nrOfCellphones);

return *this;
}

the program works now, however, is this one copying in depth? My teacher told me my last assignment did not do that.

Note that `default` is a reserved keyword in c++. I'd suspect your code shown here won't compile properly at all. — πάντα ῥεῖ, Feb 28 '16 at 13:23
It is detecting that your deletion was invalid. For example, you might be double deleting something. Not really enough code here to tell; for example, I have no idea what `cellphone` is. — Cody Gray - on strike, Feb 28 '16 at 13:29
Most likely you double delete all the pointers you copied with `cellphone[i] = default.cellphone[i];` (or whatever the real code is, since `default` is a keyword that may not be used there) — M.M, Feb 28 '16 at 13:55
Can it be due to my DEconstructor also deleting cellphone[i]? — Abdominator, Feb 28 '16 at 13:56
@Abdominator What about the copy constructor? You need that to work to effectively make the other 2 functions (assignment operator, destructor) work correctly. Also, if you do have a working copy constructor which does not call the assignment operator, the assignment operator could just use simple copy / swap and not be coded in the way you've coded it. — PaulMcKenzie, Feb 28 '16 at 14:16
@Abdominator What about the destructor? How is that implemented? If both copy constructor and destructor are implemented and work correctly, the assignment operator is a 4 line function. — PaulMcKenzie, Feb 28 '16 at 14:22
Your assignment operator is incorrect. It should be returning a reference, not a brand new object. But there are other issues that make the assignment operator faulty. — PaulMcKenzie, Feb 28 '16 at 14:25
@Abdominator Your assignment operator using copy / swap will work, as long as you return a reference, not an object. If after you returned a reference, your teacher says "it does not create a deep copy", get a new teacher, for your sake. Your teacher would be just plain wrong, it isn't even an opinion. — PaulMcKenzie, Feb 28 '16 at 14:40

score 1 · Accepted Answer · edited May 23 '17 at 10:28

Your assignment operator should be returning a CellPhoneHandler&, not a CellPhoneHandler object. By returning an object, you're invoking (unnecessarily) the copy constructor.

In addition, your assignment operator fails to check for self-assignment (assigning a CellPhoneHandler object to itself). A self-assignment will wind up deleting the object's data, and then attempt to copy from a memory area that was deleted.

The assignment operator will also fail if the call to new[] throws an exception. You're changing your object's internals before you've issued a call to new[], thus corrupting the object if something goes wrong and new[] throws an exception.

However, instead of writing all of this code to implement the assignment operator, you could have leveraged the copy constructor and destructor to implement the assignment operator using the copy / swap idiom:

#include <algorithm>
//...
CellPhoneHandler& CellPhoneHandler:: operator=(const CellPhoneHandler &original)
{
    CellPhoneHandler temp(original);
    std::swap(temp.nrOfCellphones, nrOfCellphones);
    std::swap(temp.cellphone, cellphone);
    return *this;
}

This now uses the copy constructor by creating a temporary object, and just swapping out the temp object's internals with the current object's internals. Then the temporary object dies off.

This is exception safe (since if anything goes wrong with constructing temp, nothing happens to the original object), and no need to check for self assignment (you can do that for a probable optimization, but no need to actually perform this check, unlike your original attempt of the assignment operator).

assignment operator in class, _ASSERTE(_BLOCK_TYPE_IS_VALID(pHead->nBlockUse));

1 Answers1