recursive call in copy constructor

Question

I implemented a class following the rule of three, and I am getting a crash. Upon debugging I have came to the conclusion that the copy constructor is calling itself repeatedly instead of calling the equality operator. Why is this so? Shouldn't it call the equality operator?

#include <iostream>
#include <deque>
#include <cstdlib>
#define LENGTH 128

typedef struct tDataStruct
{

char strA[LENGTH];

char strB[LENGTH];
int nNumberOfSignals;
double* oQueue;

tDataStruct()
{
    nNumberOfSignals = 0;
    //oQueue = NULL;
    memset(strA, 0, LENGTH);
    memset(strB, 0, LENGTH);
}

~tDataStruct()
{
    if (NULL != oQueue)
    {
        delete[] oQueue;
        oQueue = NULL;
    }
}

tDataStruct(const tDataStruct& other) // copy constructor
{
    if (this != &other)
    {
        *this = other;
    }

}
tDataStruct& operator=(tDataStruct other) // copy assignment
{
    if (this == &other)
    {
        return *this;
    }
    strncpy_s(strA, other.strA, LENGTH);
    strncpy_s(strB, other.strB, LENGTH);
    nNumberOfSignals = other.nNumberOfSignals;
    if (NULL != oQueue)
    {
        delete[] oQueue;
        oQueue = NULL;
    }
    if (other.nNumberOfSignals > 0)
    {
        //memcpy(oQueue, other.oQueue, nNumberOfSignals);
    }
    return *this;
}
} tDataStruct;


int main()
{
    tDataStruct tData;

    std::deque<tDataStruct> fifo;

    fifo.push_back(tData);
}

If you used std::array<char, LENGTH> instead of char [LENGTH] and std::vector<double> instead of double* all the compiler generated members would behavior correctly. — François Andrieux
– François Andrieux, Commented Oct 10, 2018 at 13:46
This is not the standard way of doing it. Normally the assignment operator uses the copy constructor in an idiom called "Copy and Swap" (look it up). As a result of doing it this way you have a serious bug that is causing UB (delete[] oQueue is potentially called on an un-initialized variable. Thus you are randomly deleting some part of memory (not a good idea). — Loki Astari
– Loki Astari, Commented Oct 10, 2018 at 14:11

NathanOliver · Accepted Answer · 2018-10-10 13:49:20Z

7

In your copy constructor you use

*this = other; //(1)

which calls

tDataStruct& operator=(tDataStruct other)  //(2)

as other is passed by value it needs to make a copy. That then calls 1, which call 2 which then calls 1 which then calls 2 and a round and a round you'll go until the program crashes/terminates.

You need to take other by reference so you don't actually make a copy like

tDataStruct& operator=(const tDataStruct& other)

All that said you are doing this backwards. You should use the copy and swap idiom and implement your operator = by using your copy constructor.

edited Oct 10, 2018 at 13:49

answered Oct 10, 2018 at 13:42

NathanOliver

183k29 gold badges323 silver badges439 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Michael Veksler · Accepted Answer · 2018-10-10 13:52:58Z

0

Your copy constructor calls assignment:

tDataStruct(const tDataStruct& other) // copy constructor
{
    // NOTE: this redundant check is always true. 
    // Please remove the if.
    if (this != &other) 
    {
        *this = other;
    }
 }

Then, since assignment operator gets the object by value (and not by reference), a copy constructor is called in order to copy the parameter:

tDataStruct& operator=(tDataStruct other) // copy assignment
{

This is how you got mutual recursion.

Try passing by reference instead:

tDataStruct& operator=(const tDataStruct &other) // copy assignment

edited Oct 10, 2018 at 13:52

answered Oct 10, 2018 at 13:47

Michael Veksler

8,5101 gold badge24 silver badges37 bronze badges

Collectives™ on Stack Overflow

recursive call in copy constructor

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related