Rvalues and constructor calls

[R10111001]

I wrote a small program to see how rvalue reference overloads (of the copy constructor and the assignment operator) can be used to prevent the unnecessary copying of objects. The output I receive, however, eludes me a little as I get 5 destructor calls for 4 constructor calls. Do rvalues (in the form of temporary objects) not sollicit explicit constructor calls?

[coder777]

The output I receive, however, eludes me a little as I get 5 destructor calls for 4 constructor calls.

No, since there is only one destructor but multiple constructor (even generated by the compiler) 1 constructor call escaped you.
Except your program is already in an unpredictable state

[R10111001]

I don't fully understand. I agree that I am missing a constructor call somewhere, my question is precisely how this is possible. I declared my (default) constuctor as explicit, so it can't be a constructor call resulting from an implicit conversion. And why do you say my program is "already in an unpredictable state"?

[coder777]

Since you did't provide any code I have no idea what's going on in your code...

The compiler generates two constructors (if necessary):
the default constructor
the copy constructor

[R10111001]

I'll provide both the code and the output i receive. It is a small program in which a CMessage object stores and displays a C-style string:

#include <iostream>
#include <cstring>

using std::cout;

class CMessage
{
	private:			// Private members of the class
		
		char* pmessage;

	public:				// Public interface of the class
		// Public member functions
		void ShowIt(void) const	
		{						
			cout << this->pmessage;
		}

		// Class (copy) constructors/destructors
		explicit CMessage (const char* text = "Default message.\n")	
		{
			cout << "Constructor called.\n";				
			pmessage = new char[strlen(text) + 1];			
			strcpy_s(pmessage, strlen(text) + 1, text);	
		}

		CMessage(const CMessage& aMessage)		
		{										
			cout << "Copy constructor called.\n";

			pmessage = new char[strlen(aMessage.pmessage) + 1];			
			strcpy_s(pmessage, strlen(aMessage.pmessage) + 1, aMessage.pmessage);
		}

		CMessage(CMessage&& aMessage)			
		{
			cout << "Move copy constructor called.\n";	
															 
			pmessage = aMessage.pmessage;					
			aMessage.pmessage = nullptr;					
		}													

		~CMessage()	// Class destructor
		{
			cout << "Destructor called.\n";

			delete [] pmessage;
		}

		// Operator overloading
		CMessage& operator = (const CMessage& aMessage)	
		{
			cout << "Assignment operator overload called.\n";
			
			if(this == &aMessage)	 
				return *this;		

			if(this->pmessage != nullptr)	
				delete [] pmessage;

			pmessage = new char[strlen(aMessage.pmessage) + 1];			
			strcpy_s(pmessage, strlen(aMessage.pmessage) + 1, aMessage.pmessage);

			return *this;		
		}					

		CMessage& operator = (CMessage&& aMessage)		 
		{												 
			cout << "Move assignment operator overload called.\n";	

			delete [] pmessage; 
			pmessage = aMessage.pmessage; 
			aMessage.pmessage = nullptr;	

			return *this;
		}
		

		CMessage operator + (const CMessage& aMessage) const 
		{
			cout << "Add operator overload called.\n";
			
			size_t length = strlen(this->pmessage) + strlen(aMessage.pmessage) + 1;	

			CMessage Message(new char[length]);					
			strcpy_s(Message.pmessage, length, this->pmessage);	
			strcat_s(Message.pmessage, length, aMessage.pmessage);	
			
			return Message;		
		}
};	
int main(void)
{
	CMessage motto1("I am Ozymandias, king of kings. ");
	CMessage motto2("Look on my works ye mighty and despair.\n");
	CMessage motto3;

	cout << "\nExecuting: motto3 = motto1 + motto2.\n";

	motto3 = motto1 + motto2;

	cout << "Done!\n\n";

	motto3.ShowIt();
	cout << "\n";
	
	return 0;
}

The output I receive is:

Constructor called.
Constructor called.
Constructor called.

Executing: motto3 = motto1 + motto2.
Add operator overload called.
Constructor called.
Move copy constructor called.
Destructor called.
Move assignment operator overload called.
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

I can explain the constructor call after the add operator overload call. However, two destructor calls follow this one constructor call. The move assignment operator overload is called to initialize a temporary (rvalue) object with the temporary object returned by the add operator overload, explaining one destructor call (the local object that goes out of scope). However, another temporary object must be in existence at this time as a destination object and, after that, as a source object for the final assignment.

I hope I made things clearer now; there must be a constructor call somewhere that I'm not picking up on.

[coder777]

I hope I made things clearer now; there must be a constructor call somewhere that I'm not picking up on.

Well:

Constructor called.
Constructor called.
Constructor called.

Executing: motto3 = motto1 + motto2.
Add operator overload called.
Constructor called.
Move copy constructor called.



Destructor called.
Move assignment operator overload called.
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

I find it somewhat surprising that the move constructor is called.

By the way: line 85 is invalid

[andywestken]

I think the reason for the imbalance between constructors and destructors is the move semantics. We really need input from someone who's looked into this properly, but when you move an object, there's noting left behind which needs destructing.

I think the move constructor makes sense to the compiler here as you're not using motto1 or motto2 after the addition; the fact than the move leaves one of the objects a dried out hisk is not relevant to what happens later.

Andy

Edit: should have read further down the thread before posting.... Andy

[coder777]

andywestken wrote:

I think the reason for the imbalance between constructors and destructors is the move semantics.

Hm, there's no imbalance.

The number of constructors: 5
The number of destructors: 5

[andywestken]

Good point.

I made the mistake of accepting "I get 5 destructor calls for 4 constructor calls." at face value when I should have fully read the rest of the thread.

But I'm not surprised that the move constructor is called. It's called when returning Message from operator=. As Message is going out of scope, its state doesn't matter anymore.

If I comment out the move constructor and assignment operator, the copy constructor and assignment operator are called instead.

Andy

[coder777]

without the bug on line 85 it shows the expected result:

Constructor called.
Constructor called.
Constructor called.

Executing: motto3 = motto1 + motto2.
Add operator overload called.
Constructor called.
Move assignment operator overload called.
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

[andywestken]

Well, when I fix the bug my output still shows the move constructor.

But that's because I build debug when prototyping, so the optimizer hadn't stripped out the superfluous constructor. I get consistent results with you when I build release.

So I was getting what I expected! (i.e. there is a construction associated with the return which is optimized out in the release build.)

Andy

PS the constructor elision is consistent with the normal copy constructor, I see. For a moment I thought they might be different.

[R10111001]

Thank you both for your replies. I was working on this early in the morning, though I still cannot believe I overlooked the fact that the move copy constructor is itself a constructor. I don't see, however, anything wrong with line 85. How exactly is it invalid?

[andywestken]

How exactly is it invalid?

If you walk through your operator= you'll see that you:

1. calculate the total length of the two strings which need to be concatenated (ok.)

2. allocate an uninitialized buffer using new and pass it to the CMessage constructor (not ok.)

3. the CMessage constructor then
3.1 calls strlen on the unitialized buffer; so the length will be determined by wherever strlen finds a random null char, which may or may not be located within the buffer you allocated (not ok.)
3.2 allocates another buffer, using this random length, so you've leaked the buffer you allocated in operator= (not ok.)
3.3 copies a random number (as found in 3.1) of random chars (from the uninitialized memory you passed to the constructor) into the new buffer (doubly not ok.)

4. when the constructor returns you then copy the strings into the new buffer, not the one you first allocated, which might be long enough; but then again, it might not be, as it's a random length (not ok.)

(Apart from anything else, it's bad form to copy data into someone else's buffer.)

When I build your code in release mode with Visual C++ it crashes:

The instruction at "0x7c9108f32 referenced memory at "0x666f2067". The memory could not be "written".

A rather blunt fix is:

        CMessage operator + (const CMessage& aMessage) const 
        {
            cout << "Add operator overload called.\n";
            
            // calculate length of concatentated string, as before
            size_t length = strlen(this->pmessage) + strlen(aMessage.pmessage) + 1;	

            // allocate a temp buffer and copy strings into it
            char* psz = new char[length];
            strcpy_s(psz, length, this->pmessage);	
            strcat_s(psz, length, aMessage.pmessage);

            // construct new message using new string (the constuctor allocates
            // another, new buffer and copies the string into it.)
            CMessage Message(psz);

            // free temp buffer so we don't leak
            delete [] psz;

            // return new message          
            return Message;		
        }

(A "non-blunt" fix would require something along the lines of adding an append method to Message, so it can be made responsible for the string concatention, rather than doing the work for the message externally.)

Andy

PS while debugging the code I noticed the constructor elision still occured; but as the program crashed it wasn't a lot of use.

[andywestken]

A bit more....

Adding output to the constructor and operator= so they report the lengths that strlen returned, the output for the release (optimized) build is:

Constructor called.
length #2 = 32
Constructor called.
length #2 = 40
Constructor called.
length #2 = 17

Executing: motto3 = motto1 + motto2.
Add operator overload called.
length #1 = 73
Constructor called.
length #2 = 3
Move assignment operator overload called.
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

So here you're copying 73 chars into a (newly allocated) buffer of (3 + 1) chars.

Andy

[coder777]

Yes, the uninitialized memory provided on line 85 is the problem.
The other problem I'm havin is this:

Constructor called.
Constructor called.
Constructor called.

Executing: motto3 = motto1 + motto2.
Add operator overload called.
Constructor called.  <----- c'tor Message
Move copy constructor called.  <----- move ctor of a temporary object. (re)moves the data from Message
Destructor called.  <----- the data is destroyed
Move assignment operator overload called.  <----- assigns Message, but the data is gone
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

How could that work?

[R10111001]

Thanks again for your detailed replies. You guys are 100% right, the uninitialized memory on line 85 is a problem. However, asides from Andy's 'blunt' fix, I see another rather simple way to solve this:

		CMessage operator + (const CMessage& aMessage) const 
		{
			cout << "Add operator overload called.\n";
			
			size_t length = strlen(this->pmessage) + strlen(aMessage.pmessage) + 1;	

			CMessage Message;
			delete [] Message.pmessage;	// Release memory assigned by default constructor
			Message.pmessage = new char[length];					
			strcpy_s(Message.pmessage, length, this->pmessage);	
			strcat_s(Message.pmessage, length, aMessage.pmessage);
			
			return Message;		
		}

As for the 'flow' of the program, as far as I understand my own program the following should be the case:

Constructor called.
Constructor called.
Constructor called.

Executing: motto3 = motto1 + motto2.
Add operator overload called.
Constructor called.  <----- c'tor for local object Message
Move copy constructor called.  <----- move c'tor of a temporary object. moves the data from local object Message to another temp object (rvalue)
Destructor called.  <----- Local object Message is first set to nullptr and then destroyed
Move assignment operator overload called.  <----- Moves the data from the temp object (rvalue) to Motto3.pmessage
Destructor called.
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called.
Destructor called.
Destructor called.

[andywestken]

I've added further logging to the classes so the instances identify who they are:

[id, how created, who created from, name]

id               = sequential assigned ID number (in order of construction)
how created      = init, copy, move
who created from = id of instanced copied/moved from (-1 if from nowhere)
name             = given name, if provided, or "anon"

Here Message is now called mottoTemp, and motto1, 2, 3 are mottoA, B, C (I made the change so I didn't have to worry about keeping the names in step with the inst IDs.)

The answer is that the move assignment operator is not applied to mottoTemp (Message), but to the temp created by the move construction.

Sequence for unoptimized build:

Constructor called [0, init, -1, mottoA]
length #2 = 32
Constructor called [1, init, -1, mottoB]
length #2 = 40
Constructor called [2, init, -1, mottoC]
length #2 = 17

Executing: mottoC = mottoA + mottoB.
Add operator overload called [0, init, -1, mottoA] add [1, init, -1, mottoB]
length #1 = 73
Constructor called [3, init, -1, mottoTemp] <------ c'tor mottoTemp
length #2 = 72
Move copy constructor called [4, move, 3, anon]
    from [3, init, -1, mottoTemp] <----- move c'tor of anon temp (moves data from mottoTemp to anon)
Destructor called [3, init, -1, mottoTemp] <----- destroy mottoTemp (the data is not destroyed, as it's been moved elsewhere)
Move assignment operator overload called [2, init, -1, mottoC]
    from [4, move, 3, anon]  <----- assigns from anon temp, not mottoTemp, so data not gone
Destructor called [4, move, 3, anon] <----- destroy anon temp (data already gone...)
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called [2, init, -1, mottoC]
Destructor called [1, init, -1, mottoB]
Destructor called [0, init, -1, mottoA]

Andy

PS Sequence for optimized build

Here the anon temp is eliminated (optimized out) and the move is from mottoTemp.

Constructor called [0, init, -1, mottoA]
length #2 = 32
Constructor called [1, init, -1, mottoB]
length #2 = 40
Constructor called [2, init, -1, mottoC]
length #2 = 17

Executing: mottoC = mottoA + mottoB.
Add operator overload called [0, init, -1, mottoA] add [1, init, -1, mottoB]
length #1 = 73
Constructor called [3, init, -1, mottoTemp]
length #2 = 72
Move assignment operator overload called [2, init, -1, mottoC]
     from [3, init, -1, mottoTemp]
Destructor called [3, init, -1, mottoTemp]
Done!

I am Ozymandias, king of kings. Look on my works ye mighty and despair.

Destructor called [2, init, -1, mottoC]
Destructor called [1, init, -1, mottoB]
Destructor called [0, init, -1, mottoA]

[andywestken]

PPS the code I used to create o/p in prev post.

Andy

#include <iostream>
#include <cstring>

//#define DISABLE_MOVE
#define FIX_BUG

using std::cout;

class CMessage
{
    private:            // Private members of the class
        char* pmessage;

        const char* howMade;
        int         instId;
        int         instFromId;
        const char* instName;

        static int nextInstId;

    public:             // Public interface of the class
        // Public member functions
        void ShowIt(void) const
        {
            cout << this->pmessage;
        }

        // Class (copy) constructors/destructors
        explicit CMessage (const char* name, const char* text = "Default message.\n")
        : pmessage(nullptr), howMade("init"), instId(nextInstId++), instFromId(-1), instName((name == nullptr) ? "null" : name)
        {
            cout << "Constructor called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "]\n";
            size_t length = strlen(text);
            cout << "length #2 = " << length << "\n";
            pmessage = new char[length + 1];
            strcpy_s(pmessage, length + 1, text);
        }

        CMessage(const CMessage& aMessage)
        : pmessage(nullptr), howMade("copy"), instId(nextInstId++), instFromId(aMessage.instId), instName("anon")
        {
            cout << "Copy constructor called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "] from [" << aMessage.instId
                 << ", " << aMessage.howMade << ", " << aMessage.instFromId << ", "
                 << aMessage.instName << "]\n";

            pmessage = new char[strlen(aMessage.pmessage) + 1];
            strcpy_s(pmessage, strlen(aMessage.pmessage) + 1, aMessage.pmessage);
        }

#ifndef DISABLE_MOVE
        CMessage(CMessage&& aMessage)
        : pmessage(nullptr), howMade("move"), instId(nextInstId++), instFromId(aMessage.instId), instName("anon")
        {
            cout << "Move copy constructor called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "] from [" << aMessage.instId
                 << ", " << aMessage.howMade << ", " << aMessage.instFromId << ", "
                 << aMessage.instName << "]\n";

            pmessage = aMessage.pmessage;
            aMessage.pmessage = nullptr;
        }
#endif

        ~CMessage() // Class destructor
        {
            cout << "Destructor called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "]\n";

            delete [] pmessage;
        }

        // Operator overloading
        CMessage& operator = (const CMessage& aMessage)
        {
            cout << "Assignment operator overload called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "] from [" << aMessage.instId
                 << ", " << aMessage.howMade << ", " << aMessage.instFromId << ", "
                 << aMessage.instName << "]\n";

            if(this == &aMessage)
                return *this;

            if(this->pmessage != nullptr)
                delete [] pmessage;

            pmessage = new char[strlen(aMessage.pmessage) + 1];
            strcpy_s(pmessage, strlen(aMessage.pmessage) + 1, aMessage.pmessage);

            return *this;
        }

#ifndef DISABLE_MOVE
        CMessage& operator = (CMessage&& aMessage)
        {
            cout << "Move assignment operator overload called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "] from [" << aMessage.instId
                 << ", " << aMessage.howMade << ", " << aMessage.instFromId << ", "
                 << aMessage.instName << "]\n";

            delete [] pmessage;
            pmessage = aMessage.pmessage;
            aMessage.pmessage = nullptr;

            return *this;
        }
#endif

        CMessage operator + (const CMessage& aMessage) const 
        {
            cout << "Add operator overload called [" << instId << ", " << howMade
                 << ", " << instFromId << ", " << instName << "] add [" << aMessage.instId
                 << ", " << aMessage.howMade << ", " << aMessage.instFromId << ", "
                 << aMessage.instName << "]\n";

            size_t length = strlen(this->pmessage) + strlen(aMessage.pmessage) + 1;
            cout << "length #1 = " << length << "\n";

#ifndef FIX_BUG
            CMessage Message(new char[length]);
            strcpy_s(Message.pmessage, length, this->pmessage);
            strcat_s(Message.pmessage, length, aMessage.pmessage);
#else
            char* psz = new char[length];
            strcpy_s(psz, length, this->pmessage);
            strcat_s(psz, length, aMessage.pmessage);
            CMessage mottoTemp("mottoTemp", psz); // was Message
            delete [] psz;
#endif

            return mottoTemp;
        }
};

/*static*/ int CMessage::nextInstId = 0;

int main(void)
{
    // renamed from motto1, 2, 3 as numbers used for inst IDs
    CMessage mottoA("mottoA", "I am Ozymandias, king of kings. ");
    CMessage mottoB("mottoB", "Look on my works ye mighty and despair.\n");
    CMessage mottoC("mottoC");

    cout << "\nExecuting: mottoC = mottoA + mottoB.\n";

    mottoC = mottoA + mottoB;

    cout << "Done!\n\n";

    mottoC.ShowIt();
    cout << "\n";

    return 0;
}

[andywestken]

However, asides from Andy's 'blunt' fix, I see another rather simple way to solve this: ...

While that approach does fix the problem, it involves one instance working on another instance's data.

Instances should look after their own data, not have it meddled with by someone else.

As the code stands it is a bit of a moot point; in actual practice (rather than in a toy program, for practice) the char* member should be replaced by one which is a std::string.

As of C++11, std::string provides move forms of the copy constructor and assigment operator:

string (string&& str) noexcept;
http://www.cplusplus.com/reference/string/string/string/

string& operator= (string&& str) noexcept;
http://www.cplusplus.com/reference/string/string/operator=/

so there's not a lot left for you to do.

Andy

[R10111001]

I agree that it is of course easier to just use string objects, but I'm still learning and it's good practice to toy around with c-style strings. In any case, thanks to you both for your help. I'll mark the topic as solved!