0

I am trying to get my head around the Rule of 5.

I have a class Renderable, which defines a custom destructor, so it seemed like a good candidate for the Rule of 5. This class creates some resources in its constructor, so my first thought was that I should prevent copying:

class Renderable {

public:

    Renderable(const Sprite&) {
        // Allocate resources
    }

    ~Renderable() {
        // Free resources
    }

    // Prevent copying
    Renderable(const Renderable& other) = delete;

}

I have another class, Unit, which creates a Renderable in the initializer list of its constructor:

class Unit {

public:

    Unit(const Sprite& sprite) :
            renderable(Renderable(sprite)) {}

private:

    Renderable renderable;

}

I would expect this to call the regular Renderable constructor, but instead I get the error:

Renderable::Renderable(const Renderable &)': attempting to reference a deleted function

Why is this trying to call the copy constructor?

I even tried added debug lines to the copy constructor, but nothing is printed:

Renderable(const Renderable& other) : sprite(other.sprite) {
    std::cout << "copy constructor";
}
Dan
  • 1,198
  • 4
  • 17
  • 34
  • Can we get a [mre]? – NathanOliver Aug 22 '19 at 21:32
  • 2
    Because you explicitly called the copy constructor in a way that it can be elided, but only if it is not deleted... – Chris Dodd Aug 22 '19 at 21:33
  • 1
    @ChrisDodd guaranteed copy elision works even if the move and copy operators are deleted: http://coliru.stacked-crooked.com/a/3b362cf2fe731f71 – NathanOliver Aug 22 '19 at 21:39
  • 1
    Wouldn't `Unit::Unit(const Sprite& sprite) : renderable(sprite) {}` work? – Ted Lyngmo Aug 22 '19 at 21:45
  • @NathanOliver: in that example you get a default move ctor, as there is no user-defined destructor (compared to the OP's code) – Chris Dodd Aug 22 '19 at 22:21
  • @ChrisDodd But deleting that move ctor doesn't matter: http://coliru.stacked-crooked.com/a/519597216ab37af9 – Ted Lyngmo Aug 22 '19 at 22:26
  • @ChrisDodd No you don't. deleting the copy constructor stops all move and the copy assignment from being generated. see: https://stackoverflow.com/questions/3734247/what-are-all-the-member-functions-created-by-compiler-for-a-class-does-that-hap – NathanOliver Aug 22 '19 at 22:34
  • Which C++ version are you using? – Ted Lyngmo Aug 22 '19 at 22:57
  • @TedLyngmo The default in VS 2017, which I believe is C++14. – Dan Aug 23 '19 at 08:40
  • If you manually select a C++ version to make sure which version you use, you can then add the version tag to your questions which makes it easier for people to help. The accepted solution obviously works in both C++14 and 17 (since it's avoiding the temporary in C++14), but the answer doesn't explain why you get a compilation error in C++14 while your original code would work fine in C++17 (as @NathanOliver has mentioned and proven in links to code). – Ted Lyngmo Aug 23 '19 at 10:49

3 Answers3

4

First, Renderable(sprite) creates a Renderable. Then you try to construct renderable with that Renderable. Conceptually, what could that use other than a copy constructor?

Why are you creating a Renderable to initialize renderable? That step is not needed and won't work because you have no copy constructor. You've specifically said that you don't want code that conceptually uses a copy constructor to work.

David Schwartz
  • 179,497
  • 17
  • 214
  • 278
  • 1
    *What could that use other than a copy constructor?* C++17's guaranteed copy elision says the direct constructor: http://coliru.stacked-crooked.com/a/b8874fa5edf90564 – NathanOliver Aug 22 '19 at 21:37
  • I guess I should clarify that I mean conceptually, not in actual execution. – David Schwartz Aug 22 '19 at 21:38
  • 1
    Even conceptually. You have a prvalue, which is not actually creating a temporary. – NathanOliver Aug 22 '19 at 21:38
  • 1
    @NathanOliver probably they are using a pre-C++17 compiler – M.M Aug 22 '19 at 22:17
  • I thought it was necessary to call the constructor explicitly, to provide a value to `renderable`. You're absolutely right, though, that step is not needed. – Dan Aug 23 '19 at 08:45
  • 1
    @Dan The value you want to provide to `renderable`'s constructor is `sprite`. Since this is an initialization list, it's a given that you're constructing the object you're initializing. – David Schwartz Aug 23 '19 at 14:02
3

In addition to what others have said, I think you meant to write:

Unit::Unit(const Sprite& sprite) :
    renderable(sprite) {}

This invokes the converting constructor Renderable(const Sprite&) to initialise renderable directly, no copying involved.

Live demo

Paul Sanders
  • 24,133
  • 4
  • 26
  • 48
  • This is exactly right, thank you. I did not know that this would implicitly call the `Renderable` constructor. – Dan Aug 23 '19 at 08:43
1

Why is this trying to call the copy constructor?

Because

  renderable(Renderable(sprite)) {}

This constructs a temporary Renderable object, and then uses it to construct the renderable class members. That would be a

I even tried added debug lines to the copy constructor, but nothing is printed:

This is because this is one of the situations where compilers are permitted to do copy-elision. Even though the compiler optimizes away a temporary+copy construction, the constructor must still exist. Something about the class causes the default copy constructor to be deleted. There could be several reasons for that, but you did not provide sufficient information about your class in order to determine what that reason might be.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • It's explicitly deleted: `Renderable(const Renderable& other) = delete;` – Ted Lyngmo Aug 22 '19 at 21:38
  • @TedLyngmo Doesn't matter in C++17: http://coliru.stacked-crooked.com/a/3b362cf2fe731f71 – NathanOliver Aug 22 '19 at 21:41
  • @NathanOliver It was a comment on "_Something about the class causes the default copy constructor to be deleted._" but perhaps I misunderstood. – Ted Lyngmo Aug 22 '19 at 21:42
  • @TedLyngmo The copy constructor wasn't being deleted when I added the debug lines. – Dan Aug 23 '19 at 08:48
  • @Dan No, but then you didn't get the error about the deleted copy constructor either. The compiler figured out that it could actually elide the copying all together (which is why you didn't see your `"copy constructor"` message). – Ted Lyngmo Aug 23 '19 at 10:10