Sign in to follow this  
noodleBowl

C++ Copy, move, and assignment operators with a derived class

Recommended Posts

When it comes to the copy, move, and assignment operators of a child class how is it supposed to look?

If this is the implementation of my parent class

//Default constructor
Buffer::Buffer()
{
	buffer = nullptr;
}

//Copy constructor
Buffer::Buffer(const Buffer& other)
{
	buffer = other.buffer;
	buffer->AddRef();
}

//Move constructor
Buffer::Buffer(Buffer&& other)
{
	buffer = other.buffer;
	buffer->AddRef();
	other.release(); //Free the buffer resource of the other instance
}

//Destructor
Buffer::~Buffer()
{
	release();
}

//Copy assignment
Buffer& Buffer::operator=(const Buffer& other)
{
	if (&other != this)
	{
		release(); //Free the buffer of this instance

		buffer = other.buffer;
		buffer->AddRef();
	}

	return *this;
}

//Move assignment
Buffer& Buffer::operator=(Buffer&& other)
{
	if (&other != this)
	{
		release(); //Free the buffer of this instance

		buffer = other.buffer;
		buffer->AddRef();

		other.release(); //Free the buffer of the other instance
	}

	return *this;
}

And this is the implementation of my child class. Is this correct?

I'm really just wondering about the at the copy, move, and I'm not too sure about the assignment operators

//Default constructor
VertexBuffer::VertexBuffer() : Buffer()
{
}

//Copt constructor
VertexBuffer::VertexBuffer(const VertexBuffer& other) : Buffer(other)
{
}

//Move constructor
VertexBuffer::VertexBuffer(VertexBuffer&& other) : Buffer(other)
{
}

//Destructor
VertexBuffer::~VertexBuffer()
{
}

//Not sure how to handle the copy/move operators.
//Do I treat them as there own operators and copy the Buffer assignment operators code?
VertexBuffer& VertexBuffer::operator=(const VertexBuffer& other)
{
}

VertexBuffer& VertexBuffer::operator=(VertexBuffer&& other)
{
}

 

Share this post


Link to post
Share on other sites

Deriving from a copyable base class is a bit of a code smell IMHO (but so is deriving from concrete classes in general :) ). FWIW I don't know if I've ever needed to solve this problem... 

As for how to call the base class assignment operator, you can always do something like:

*static_cast<Base*>(this) = other 

Share this post


Link to post
Share on other sites
8 minutes ago, Hodgman said:

Deriving from a copyable base class is a bit of a code smell IMHO (but so is deriving from concrete classes in general ). FWIW I don't know if I've ever needed to solve this problem... 

As for how to call the base class assignment operator, you can always do something like:

*static_cast<Base*>(this) = other 

Just make it very explicit: Base::operator=(other);

Share this post


Link to post
Share on other sites

A copy assignment operator should always look like this.

VertexBuffer& VertexBuffer::operator=(const VertexBuffer other)
{
	this->swp(other);
	return *this;
}

Of course, you need to implement your swap() member function, but that should be fairly trivial.  See, the magic of this is it implements the copy assignment operator in terms of the copy constructor, which is generally the expected behaviour, so you only write the meaty function once.  Even your base class should implement the operator this way.  Trace it through, you'll see what I mean.

Also, why would you create a class hierarchy without a virtual destructor in the base class?  That smells of a misuse of inheritance.

Share this post


Link to post
Share on other sites
16 minutes ago, Bregma said:

A copy assignment operator should always look like this.


VertexBuffer& VertexBuffer::operator=(const VertexBuffer other)
{
	this->swp(other);
	return *this;
}

Of course, you need to implement your swap() member function, but that should be fairly trivial.  See, the magic of this is it implements the copy assignment operator in terms of the copy constructor, which is generally the expected behaviour, so you only write the meaty function once.  Even your base class should implement the operator this way.  Trace it through, you'll see what I mean.

Also, why would you create a class hierarchy without a virtual destructor in the base class?  That smells of a misuse of inheritance.

Shoudn't the argument be a reference for a general copy assignment operator?

My understanding of the default copy assignment operator was to call the base copy assignment operator first and then the copy assignment operators of each of the object's members.

Edited by matt77hias

Share this post


Link to post
Share on other sites

Why are you using inheritance here? Do you have anywhere in your code a pointer or reference to a buffer that might be a vertex buffer or a different type of buffer, and you'll only know at run time? That would be about the only situation in which I'd use inheritance.

Inheritance is easily abused. Don't use it until you really need it.

 

Share this post


Link to post
Share on other sites
7 hours ago, Hodgman said:

Deriving from a copyable base class is a bit of a code smell IMHO (but so is deriving from concrete classes in general )

I actually don't mean for the Buffer class to be concrete I just don't know how to make it not since there are no methods that need to be implemented for the child

//Parent class
class Buffer
{
public:
	Buffer();
	Buffer(const Buffer& other);
	Buffer(Buffer&& other);
	virtual ~Buffer();
	Buffer& operator=(const Buffer& other);
	Buffer& operator=(Buffer&& other);

private:
	ID3D11Buffer *buffer;
	void release(); //Releases the buffer member variable and cleans it up
};


//VertexBuffer child class, other versions would be ConstantBuffer or IndexBuffer
class VertexBuffer : public Buffer
{
public:
	VertexBuffer();
	VertexBuffer(const VertexBuffer& other);
	VertexBuffer(VertexBuffer&& other);
	~VertexBuffer();
	VertexBuffer& operator=(const VertexBuffer& other);
	VertexBuffer& operator=(VertexBuffer&& other);
};

 

7 hours ago, matt77hias said:

Just make it very explicit: Base::operator=(other);

Do I need to set anything to the left hand side when I do this? or is it really just:

//Vertex Buffer copy assignment opt
VertexBuffer& VertexBuffer::operator=(const VertexBuffer& other)
{
	if (&other != this)
	{
		Buffer::operator=(other);
	}

	return *this;
}

//Vertex Buffer move assignment opt
VertexBuffer& VertexBuffer::operator=(VertexBuffer&& other)
{
	if (&other != this)
	{
		Buffer::operator=(other);
	}
  
	return *this;
}

 

1 hour ago, Bregma said:

why would you create a class hierarchy without a virtual destructor in the base class?

I posted the implementation file contents before. Intellisense gets mad at me if use the virtual keyword there, but its in the header file :)

 

15 minutes ago, alvaro said:

Why are you using inheritance here? Do you have anywhere in your code a pointer or reference to a buffer that might be a vertex buffer or a different type of buffer, and you'll only know at run time? That would be about the only situation in which I'd use inheritance.

Inheritance is easily abused. Don't use it until you really need it.

Currently I really don't have a point where I need a container of Buffers. For now I will definitely know that I need a VertexBuffer here or I should use a IndexBuffer there. I guess I really just have a base class, because regardless of whether my instance object is a VertexBuffer, ConstantBuffer, or XXXXBuffer they are still all buffers at the core and all have that ID3D11Buffer *buffer member variable in common

Edited by noodleBowl

Share this post


Link to post
Share on other sites
12 minutes ago, noodleBowl said:

I actually don't mean for the Buffer class to be concrete I just don't know how to make it not since there are no methods that need to be implemented for the child

If the derived class destructor needs to be called as well, when the object is destroyed via the base class destructor, you need a virtual destructor in the base class.

12 minutes ago, noodleBowl said:

Do I need to set anything to the left hand side when I do this? or is it really just:

That is ok if you have no data to copy for the VertexBuffer itself. For the move assignment operator, you need to use std::move(other), otherwise you will invoke the copy instead of move assignment operator of the base class.

If this is literally the implementation you want, just use = default for both.

Edited by matt77hias

Share this post


Link to post
Share on other sites
13 minutes ago, noodleBowl said:

I posted the implementation file contents before. Intellisense gets mad at me if use the virtual keyword there, but its in the header file :)

I don't know why Intellisense gets mad, but normally you shouldn't care about Intellisense. The only one who really knows and is allowed to complain is your compiler (output window).

Share this post


Link to post
Share on other sites
13 minutes ago, noodleBowl said:

Currently I really don't have a point where I need a container of Buffers. For now I will definitely know that I need a VertexBuffer here or I should use a IndexBuffer there. I guess I really just have a base class, because regardless of whether my instance object is a VertexBuffer, ConstantBuffer, or XXXXBuffer they are still all buffers at the core and all have that ID3D11Buffer *buffer member variable in common

 

You don't need to inherit to share functionality. Composition will be better in the vast majority of situations. So both VertexBuffer and ConstantBuffer can have a member of type Buffer (or MemoryBuffer, or RawBuffer... Using good names is important) and share functionality that way.

 

Share this post


Link to post
Share on other sites
35 minutes ago, matt77hias said:

I don't know why Intellisense gets mad

Intellisesne might be the wrong word here. Its whichever entity is responsible putting a red squiggly under things. The linter?

30 minutes ago, alvaro said:

 

You don't need to inherit to share functionality. Composition will be better in the vast majority of situations. So both VertexBuffer and ConstantBuffer can have a member of type Buffer (or MemoryBuffer, or RawBuffer... Using good names is important) and share functionality that way.

 

I kind of lied here, completely forgot what I was trying to do. I actually do have a spot where I could use a vector of Buffers. I have this factory called BufferModule which has methods like createVertexBuffer, createIndexBuffer, etc. All of those buffer classes (VertexBuffer, IndexBuffer, etc) are derived from the base Buffer. So in order to hold all of these I need a member variable like

std::vector<Buffer*> bufferObjects;

Cause if I were to just a normal vector of objects, my child buffer class  would be sliced when I would try to do something like this right?

//Vector of the parent class Buffer
std::vector<Buffer> bufferObjects;

//Put the child class VertexBuffer into the vector
VertexBuffer vertexBuffer;
graphicsDevice->device->CreateBuffer(&bufferDescription, NULL, &vertexBuffer.buffer);
buffers.push_back(vertexBuffer); // Not good... Just lost all of the info that was not specifically a part of the parent class

So to fix this I would need to do

//Vector of the parent class Buffer. Using a pointer to prevent the object slice
std::vector<Buffer*> bufferObjects;

VertexBuffer vertexBuffer = new VertexBuffer(); //Need to use new here otherwise when this goes out of scope all of our data goes bye-bye
graphicsDevice->device->CreateBuffer(&bufferDescription, NULL, &vertexBuffer.buffer);
buffers.push_back(&vertexBuffer); //Hurray! No object slicing happens

Only problem I have here is that I really don't like the use of the new keyword :(

Share this post


Link to post
Share on other sites
1 hour ago, alvaro said:

 

You don't need to inherit to share functionality. Composition will be better in the vast majority of situations. So both VertexBuffer and ConstantBuffer can have a member of type Buffer (or MemoryBuffer, or RawBuffer... Using good names is important) and share functionality that way.

 

Note that private inheritance is one implementation of composition :P

Share this post


Link to post
Share on other sites
30 minutes ago, alvaro said:

Yes, it's the one with the confusing syntax. :)

And the confusing defaults. Why use public always as default for struct and use private always as default for class? This makes no sense for inheritance. But on the other hand if the syntax is not explicit, one asks for trouble sooner or later.

Share this post


Link to post
Share on other sites

Types should usually either have value semantics or reference semantics, but not both. Value types can be used as reference types occasionally (what C# would consider "boxing"), but not vice versa.

A polymorphic assignment operator has properties of both and is definitely a code smell. Instead, try creating a reference type that can be owned by a value type. This gives you the best of both worlds, and allows you to use pImpl and other helpful patterns. The stricter semantic dichotomy also makes the code easier to work with and allows you to avoid a lot of these dark corners of C++ :)

Share this post


Link to post
Share on other sites
5 hours ago, matt77hias said:

Shoudn't the argument be a reference for a general copy assignment operator?

No.  Trace through the operation of the code I wrote.  See how the copy constructor is used to implement the assignment operator?  Now, isn't it elegant?

Share this post


Link to post
Share on other sites
10 minutes ago, Bregma said:

No.  Trace through the operation of the code I wrote.  See how the copy constructor is used to implement the assignment operator?  Now, isn't it elegant?

Ok I get it. Elegant: yes.

But come on, you need to look at this few lines of code twice imho to get what is happening.

Someone of your team sees your signature. Adds the "missing" reference. And then it all depends on you "swp" signature, how things would go...

Share this post


Link to post
Share on other sites
58 minutes ago, matt77hias said:

Ok I get it. Elegant: yes.

But come on, you need to look at this few lines of code twice imho to get what is happening.

Someone of your team sees your signature. Adds the "missing" reference. And then it all depends on you "swp" signature, how things would go...

I personally prefer the reference as well, since you can add an if(this != &other) before creating the temporary and swapping. Even though your swap function will have the same check, and it's semantically the same either way, no use creating a throwaway copy in that case :)

Share this post


Link to post
Share on other sites
18 hours ago, matt77hias said:

Ok I get it. Elegant: yes.

But come on, you need to look at this few lines of code twice imho to get what is happening.

Someone of your team sees your signature. Adds the "missing" reference. And then it all depends on you "swp" signature, how things would go...

I do not work with bottom-grade programmers who would alter functionality without understanding it first. 

If you absolutely must blindly follow rigid ideology and make the argument to your copy assignment operator a reference, the code would look like this.

VertexBuffer& VertexBuffer::operator=(VertexBuffer const& other)
{
	VertexBuffer tmp(other);
	this->swap(tmp);
	return *this;
}

Interestingly, modern compilers will pessimize that alternative code because they can not take advantage of copy elision like they can with pass-by-value parameters.  Your coworker's blind obedience to cargo-cult pass-by-const-reference just cost a thousand CPU cycles.  Consider giving them an opportunity to pursue a more satisfying role as a node.js front-end developer where they can cause less harm.

One of the big advantages of the copy-and-swap idiom is that it provides the strong exception safety guarantee, whereas the original code did not (again, trace it through).  You might argue against the use of exceptions in C++ (after all, we're already speaking of a level of mindless reverence at play in your place of work) but writing the most robust code as possible is usually a win in the long run.  Sure, in this trivial example you're unlikely to see an exception, but not all code is so trivial and the copy-and-swap idiom will always apply.

As for adding a self-check, consider the cost of adding and maintaining an equality operator (and corresponding inequality operator) and the cost of calling it every time and compare that to how ofter you write code that performs self-assignment.  I know I very very rarely write code that assigns an object to itself, but code that is never written costs less and is guaranteed bug-free.  Always spending extra CPU cycles and increasing the risk in code to make a single highly unlikely edge-case more efficient seems like a lose-lose tradeoff to me.

Really, other than deleting the copy assignment operator entirely or adding debug printfs, why would you ever use anything other than the two-line copy-and-swap I wrote above for all your classes?

Share this post


Link to post
Share on other sites
15 minutes ago, Bregma said:

Really, other than deleting the copy assignment operator entirely or adding debug printfs, why would you ever use anything other than the two-line copy-and-swap I wrote above for all your classes?

I use for 95% of the cases:

VertexBuffer &VertexBuffer::operator=(const VertexBuffer &buffer) = delete;

and for 4.99% of the cases:

VertexBuffer &VertexBuffer::operator=(const VertexBuffer &buffer) = default;

 

Edited by matt77hias

Share this post


Link to post
Share on other sites
6 hours ago, Bregma said:

If you absolutely must blindly follow rigid ideology

 

6 hours ago, Bregma said:

Your coworker's blind obedience to cargo-cult pass-by-const-reference

 

6 hours ago, Bregma said:

Really, other than deleting the copy assignment operator entirely or adding debug printfs, why would you ever use anything other than the two-line copy-and-swap I wrote above for all your classes?

Certainly you must see the irony among these statements?

Passing by value may often elide the copy for R-values, but not L-values. Passing by reference will always copy R-values, but L-values don't necessarily have to be copied if there are (exception-safe) optimizations that can be done during assignment. While I concede that a self-check isn't a strong case for preferring the latter approach, and I'll definitely be preferring pass-by-value in the general case going forward as it's more idiomatic, it should be obvious enough that both approaches are valid depending on the usage and implementation details of specific types.

It's a fool's errand to try an make an argument for either based on contrived usage and performance conditions, especially at the micro-optimization level. I wouldn't even entertain such an argument coming from a coworker. If this code ever does become an issue, I guarantee it's because your software has much bigger issues than assignment operators performing extra copies. But again, without real data there's no way to know.

Share this post


Link to post
Share on other sites
1 minute ago, Hodgman said:

Before I start blindly following this rigid ideology, what about cases where swapping is just as expensive as copying? 

Under no circumstances should you question my obviously superior intellect.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this  

  • Announcements

  • Forum Statistics

    • Total Topics
      628367
    • Total Posts
      2982284
  • Similar Content

    • By Kazuma506
      I am trying to recreate the combat system in the game Life is Feudal but make it more complex. The fighting system works by taking in the direction of the mouse movement and if you press the left click it will swing in that direction, though stab, overhead, left (up, down, left right) and right are the only swings that you can do. If you wanted to you could also hold the swing by holding left click so you are able to swing at the perfect moment in the battle. I want to change this so add in more swing directions but I also want to code this from scratch in Unreal. Can anyone give me any pointers or maybe a few snippets of code that work in Unreal that could help me start to implement this type of system?
       
       
    • By rXpSwiss
      Hello,
      I am sending compressed json data from the UE4 client to a C++ server made with boost.
      I am using ZLib to compress and decompress all json but it doesn't work. I am now encoding it in base64 to avoid some issues but that doesn't change a thing.
      I currently stopped trying to send the data and I am writing it in a file from the client and trying to read the file and decompress on the server side.
      When the server is trying to decompress it I get an error from ZLib : zlib error: iostream error
      My question is the following : Did anyone manage to compress and decompress data between a UE4 client and a C++ server ?
      I cannot really configure anything on the server side (because boost has its ZLib compressor) and I don't know what is wrong with the decompression.
      Any idea ?
      rXp
    • By noodleBowl
      I was wondering if someone could explain this to me
      I'm working on using the windows WIC apis to load in textures for DirectX 11. I see that sometimes the WIC Pixel Formats do not directly match a DXGI Format that is used in DirectX. I see that in cases like this the original WIC Pixel Format is converted into a WIC Pixel Format that does directly match a DXGI Format. And doing this conversion is easy, but I do not understand the reason behind 2 of the WIC Pixel Formats that are converted based on Microsoft's guide
      I was wondering if someone could tell me why Microsoft's guide on this topic says that GUID_WICPixelFormat40bppCMYKAlpha should be converted into GUID_WICPixelFormat64bppRGBA and why GUID_WICPixelFormat80bppCMYKAlpha should be converted into GUID_WICPixelFormat64bppRGBA
      In one case I would think that: 
      GUID_WICPixelFormat40bppCMYKAlpha would convert to GUID_WICPixelFormat32bppRGBA and that GUID_WICPixelFormat80bppCMYKAlpha would convert to GUID_WICPixelFormat64bppRGBA, because the black channel (k) values would get readded / "swallowed" into into the CMY channels
      In the second case I would think that:
      GUID_WICPixelFormat40bppCMYKAlpha would convert to GUID_WICPixelFormat64bppRGBA and that GUID_WICPixelFormat80bppCMYKAlpha would convert to GUID_WICPixelFormat128bppRGBA, because the black channel (k) bits would get redistributed amongst the remaining 4 channels (CYMA) and those "new bits" added to those channels would fit in the GUID_WICPixelFormat64bppRGBA and GUID_WICPixelFormat128bppRGBA formats. But also seeing as there is no GUID_WICPixelFormat128bppRGBA format this case is kind of null and void
      I basically do not understand why Microsoft says GUID_WICPixelFormat40bppCMYKAlpha and GUID_WICPixelFormat80bppCMYKAlpha should convert to GUID_WICPixelFormat64bppRGBA in the end
       
    • By HD86
      As far as I know, the size of XMMATRIX must be 64 bytes, which is way too big to be returned by a function. However, DirectXMath functions do return this struct. I suppose this has something to do with the SIMD optimization. Should I return this huge struct from my own functions or should I pass it by a reference or pointer?
      This question will look silly to you if you know how SIMD works, but I don't.
    • By pristondev
      Hey, Im using directx allocate hierarchy from dx9 to use a skinned mesh system.
      one mesh will be only the skeleton with all animations others meshes will be armor, head etc, already skinned with skeleton above. No animation, idle position with skin, thats all I want to use the animation from skeleton to other meshes, so this way I can customize character with different head, armor etc. What I was thinking its copy bone matrices from skeleton mesh to others meshes, but Im a bit confused yet what way I can do this.
       
      Thanks.
  • Popular Now