Copy constructor doesn't work

Question

I have some code that creates an array of documents. Each document object has an array of document-wide values, and an array of individual files (called lines because each is a line in the source file I'm reading from) that together have all the document data. When I attempt to add a document object to the array, it is calling my copy constructor below:

CMyDocument::CMyDocument(CMyDocument& cSourceDoc)
{
    m_lpastrFields = new CStringArray;
    m_lpacLines = new CArray<CMyLine, CMyLine>;
    int nCount;
    int nSize;
    nSize = static_cast<int>(cSourceDoc.GetFields()->GetSize());
    for (nCount = 0; nCount < nSize; nCount++)
    {
        m_lpastrFields->Add(cSourceDoc.GetFields()->GetAt(nCount));
    }
    nSize = static_cast<int>(cSourceDoc.GetLines()->GetSize());
    for (nCount = 0; nCount < nSize; nCount++)
    {
        m_lpacLines->Add(cSourceDoc.GetLines()->GetAt(nCount));
    }
    m_strDocDate = cSourceDoc.GetDocDate();
    m_nDocID = cSourceDoc.GetDocID();
    m_strDocType = cSourceDoc.GetDocType();
}

The problem is, when I try to access the documents by pulling them from the document array later, the two arrays I've copied above are empty. The seem to be initialized and have memory addresses, but they contain no data. The member variables are populated though. I'm not sure if I'm doing the copying incorrectly or if the problem is elsewhere.

EDIT: The regular constructor looks like this:

CMyDocument::CMyDocument()
{
    m_lpastrFields = new CStringArray;
}

I don't new the m_lpacLines object in this case because it is passed into the MyDocument object through a function called InitDocument. I may as well include that here. (Some unnecessary details, like the way I parse the strLine variable to extract all the values, were trimmed for brevity's sake.

void CMyDocument::InitDocument(CMyColumns* lpcColumns, CString strLine, CArray<CMyLine, CMyLine>* lpacLines)
{
    CString strValue;
    CString strComma = ",";
    int     nPos = 0;

    m_lpacLines = lpacLines;

    while (-1 != nPos)
    {
        strValue = strLine.Tokenize(strComma, nPos);
        m_lpastrFields->Add(strValue);
    }
    m_strDocDate = m_lpastrFields->GetAt(lpcColumns->GetDocDateIndex());
    CString strDocID = m_lpastrFields->GetAt(lpcColumns->GetDocIDIndex());
    m_nDocID = atoi(strDocID);
    m_strDocType = m_lpastrFields->GetAt(lpcColumns->GetDocTypeIndex());
}

And to be clear, I am newing the lpacLines object outside of the InitDocument function every time I loop through. I've already debugged this code though and everything is being assigned correctly here.

SECOND EDIT: In trying to convert these all the non-pointer member variables, I am now coming up against error C2248:'CObject::CObject' : cannot access private member declared in class 'CObject'. Upon reflection, problems like this may have been what pushed me towards using pointers in the first place.

THIRD EDIT: Here is the class declaration:

class CMyDocument
{
    public:
        CMyDocument();
        ~CMyDocument();
        CMyDocument(CMyDocument& cSourceDoc);
        void    InitDocument(CMyColumns* lpcColumns, CString strLine, CArray<CMyLine, CMyLine>* lpacLines);
        inline  CString GetDocDate(void) {return(m_strDocDate);};
        inline  int     GetDocID(void) {return(m_nDocID);};
        inline  CString GetDocType(void) {return(m_strDocType);};

        inline  CStringArray*   GetFields(void) {return(m_lpastrFields);};
        inline  CArray<CMyLine, CMyLine>*   GetLines(void) {return m_lpacLines;};
    private:
        CArray<CMyLine, CMyLine>*   m_lpacLines;
        CStringArray*   m_lpastrFields;

        CString             m_strDocDate;
        int                 m_nDocID;
        CString             m_strDocType;
};

Possibly unrelated, but why do you allocate them with `new`? You could just have two member variables `CStringArray m_fields` and `CArray m_lines` and let the implicitly-generated copy constructors do the work. — Andy Prowl, Mar 25 '13 at 21:05
I've vacillated back and forth on that, but I made them pointers because I use them in a few different places and it seems easier to keep a single copy with multiple pointers because I had some concerns about using up all my memory, as the total number of lines could potentially top one million, with each line object have an array of 70 values. But perhaps it wouldn't matter, since the duplicates wouldn't be around for long and not all at once. — Joe M, Mar 25 '13 at 21:10
It looks like *the problem is elsewhere*. Step through that copy constructor with a debugger to verify. — Drew Dormann, Mar 25 '13 at 21:13
Also, since you seem to work with pointers, make sure you're not trying to invoke a member function through a pointer that happens to be null — Andy Prowl, Mar 25 '13 at 21:15
@DrewDormann the copy constructor seems to show everything properly copied, but the next time I try to retrieve the document object from the array, its member objects are empty. — Joe M, Mar 25 '13 at 21:16
@AndyProwl Heh, I had that problem separately before I made my own copy constructor, because this is part of a loop that was initially rewriting over the same pointer until I started `new`ing it in the loop and then in the copy constructor. — Joe M, Mar 25 '13 at 21:17
My money is on a double-delete due to violation of the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three): forgetting copy assignment. Doing this without pointers would be simpler, faster, use less memory. — Mooing Duck, Mar 25 '13 at 21:22
I have an explicit destructor as well, but no assignment operator. — Joe M, Mar 25 '13 at 21:25
You might want to add the class declaration to the post as well. I see a lot of code that I would write in a slightly different way but nothing that is obviously wrong. — Timo Geusch, Mar 26 '13 at 17:45
@MooingDuck I think you're onto something. I stepped back through the code and I think I was incorrect initially. It looks like it isn't correctly populating the arrays in the first place. — Joe M, Mar 26 '13 at 23:11

score 3 · Accepted Answer · edited May 23 '17 at 11:57

3

Now that you've posted the full class definition, it is clear that you are indeed violating the Rule of Three: If you need to explicitly declare either the destructor, copy constructor or copy assignment operator yourself, you probably need to explicitly declare all three of them.

You have a copy constructor, and a destructor, but no copy assignemnt. Add these members and you should be fine.

CMyDocument& operator=(CMyDocument cSourceDoc) {
    swap(cSourceDoc);
    return *this;
}

void swap(CMyDocument& cSourceDoc) {
   using std::swap;
   swap(m_lpacLines, cSourceDoc.m_lpacLines);
   swap(m_lpastrFields, cSourceDoc.m_lpastrFields);
   swap(m_strDocDate, cSourceDoc.m_strDocDate);
   swap(m_nDocID, cSourceDoc.m_nDocID);
   swap(m_strDocType, cSourceDoc.m_strDocType);
}

Your constructor allocates memory, and makes a member point at it. Somewhere in your code you are making a copy of a CMyDocument. Since you have no copy assignment operator, the compiler uselessly made one for you, that simply copies the pointer, so that you then have two CMyDocument objects pointing at the same CArray and CStringArray. Then, when one of them is deleted, they delete the CArray and CStringArray, and the other CMyDocument is left with useless pointers that point at invalid memory. When you are attempting to use that invalid memory, sometimes, if you get lucky you'll see what used to be there. In this case, the empty CArray and CStringArray. (They empty themselves right as they are deleted). If you weren't lucky, the program would have simply crashed.

edited May 23 '17 at 11:57

Community

1
1

answered Mar 26 '13 at 23:37

Mooing Duck

64,318
19
100
158

I'd also like to add that this could probably be solved by the *Rule of Zero*. – Bartek Banachewicz Mar 27 '13 at 13:20
Yes, in another class I have an array of these document objects. It calls the copy constructor whenever I add a document to the array. I'll test this out in a couple hours. – Joe M Mar 27 '13 at 13:22
Two problems: 1) I think that second `return this` in the swap function is incorrect, since it's a void function. 2) I'm getting the following error for the operator= function: `error C2440: 'return' : cannot convert from 'CMyDocument *const ' to 'CMyDocument &'` – Joe M Mar 27 '13 at 15:18
Well, I thought this was working, because it looked like it copied correctly, but it's still crashing later on when it tries to access the array. – Joe M Mar 27 '13 at 15:50
1

Okay, I figured out the last problem. I needed to initialize the m_lpacLines object pointer to `NULL` to avoid an exception in the destructor. – Joe M Mar 27 '13 at 16:08
1

@JoeMajsterski: Sorry I had the code not quite right, but I'm glad you figured it out! – Mooing Duck Mar 27 '13 at 17:02

Copy constructor doesn't work

1 Answers1