1

I'm receiving the message "Use of deleted function" when I combine the use of an ofstream (which I later want to use to record information) and the placement of one class inside another. Here is my minimal example:

#include <iostream>
#include <unistd.h>
#include <fstream>

class Tracker {
    private:
        std::ofstream tracker_file;
    public:
        Tracker(const std::string filename) {
            tracker_file.open("tracker_file.csv", std::ios_base::app);
        }
};

class Penguin {
    private:
        Tracker p_tracker;
    public:
        Penguin(
            Tracker p_tracker
        ) : p_tracker(p_tracker) {}
};

int main()
{
    Tracker penguin_tracker = Tracker("output");
    Penguin gentoo(penguin_tracker);
    return 1;
}

I don't understand how these are related, but if I remove the intermediate class then it works, and if I remove the ofstream it works.

Mark
  • 45
  • 4
  • 4
    streams are not copyable, and `Penguin gentoo(penguin_tracker)` tries to make a copy. trying to find a dupe target – NathanOliver May 03 '22 at 13:11
  • But [streams are movable... in C++11](https://en.cppreference.com/w/cpp/io/basic_fstream/basic_fstream) and beyond.... so you can `std::move` that stream into position. – Mgetz May 03 '22 at 13:25
  • Not related to the issue, but `main` should return 0 upon success (not 1). – wohlstad May 03 '22 at 13:44

1 Answers1

2

In this line in the ctor of Penguin:

) : p_tracker(p_tracker) {}

You attempt to initalize the Tracker p_tracker data member. Since you pass it an existing Tracker instance, it attempts to use the copy constuctor.

But class Tracker does not have a copy constructor. This is the "deleted function" mentioned in the error you got.

As @NathanPierson wrote in the comment below, this is because of the deleted copy constructor of the std::ofstream member variable tracker_file (and not because of the converting constructor you defined, as I originally wrote).

You could have theoretically solved it by adding a copy ctor to Tracker, something like:

Tracker(Tracker const & other)
{
    // ...
}

However - as mentioned above class Tracker has a std::ofstream member which is non copyable.

So the question is what did you mean should happen in this case ?

On a side note: using the same name: p_tracker for both the class data member and the parameter passed to the constructor is a bit confusing and not recomended.

UPDATE: To answer the question of the OP in the comment below: If class Penguin only needs to keep a refernce to a Tracker instance, you can do the following (added "m_" prefix to members to diffrentiate them from other variables):

#include <iostream>
#include <fstream>

// No change in Tracker:
class Tracker {
private:
    std::ofstream m_tracker_file;
public:
    Tracker(const std::string filename) {
        m_tracker_file.open("tracker_file.csv", std::ios_base::app);
    }
};

// Penguin now holds a reference to Tracker:
class Penguin {
private:
    Tracker & m_tracker;
public:
    Penguin(
        Tracker & tracker
    ) : m_tracker(tracker) {}
};

int main()
{
    Tracker penguin_tracker("output");
    Penguin gentoo(penguin_tracker);
    return 0;
}

However - this solution requires you to ensure the Tracker penguin_tracker is alive as long as Tracker penguin_tracker is alive (otherwise you will have a dangling reference). In your example it is OK, just mentioned in for the general case.

Another side note: main should return 0 if all went well (not 1). See: What should main() return in C and C++?.

wohlstad
  • 12,661
  • 10
  • 26
  • 39
  • Thanks! So I intended the Penguin class gentoo to reference the Tracker class penguin_tracker (attaching a reference to it upon initialisation). Is this kind of thing possible, or would I need to make a copy? – Mark May 03 '22 at 13:27
  • Yes it's possible - I updated my answer - see above. – wohlstad May 03 '22 at 13:35
  • On the side note - this is interesting, I wondered what sensible names to use to differentiate the internal storage and the external parameter. I considered cls_x or _x for the class member and x for the parameter - what's the typical convention, or is it more case-by-case? UPDATE: Just seen the m_x convention - that's a good idea! – Mark May 03 '22 at 13:37
  • There are several conventions. I personally like `m_` for members, and `` (without prefix) for the parameter passed to initialize it. – wohlstad May 03 '22 at 13:39
  • `Tracker("output")` does not compile. https://www.godbolt.org/z/c5cWobxMo – Jabberwocky May 03 '22 at 13:40
  • @Jabberwocky - It did on MSVC, so I didn't notice the issue. Anyway thanks - I fixed the code in my answer. – wohlstad May 03 '22 at 13:42
  • @Mark: It's subjective whether it's a problem to have a ctor argument named the same as a class member. Personally, I'm fine with naming both ``. Tricks like `m_somename` or `p_somename` date back to an era before modern IDE's which offer a "Peek Definition" or similar quick lookup. – MSalters May 03 '22 at 13:44
  • The code in the Update section doesn't compile, even with Visual Studio 2019 – Jabberwocky May 03 '22 at 13:55
  • @Jabberwocky - It compiles for me on 2017. And also on msvc19 via Godbolt: https://www.godbolt.org/z/q91bEYP1r (of course you need to copy-paste the full `Tracker ` class from the question). – wohlstad May 03 '22 at 14:00
  • @Jabberwocky - I now see that in your godbolt link the `Tracker` class implementation is missing (should be copied from the question). – wohlstad May 03 '22 at 14:01
  • I will update the answer to include the complete `Tracker` class for completness. – wohlstad May 03 '22 at 14:02
  • "The reason that the default copy constructor is not available is that you defined a custom constructor: [converting constructor]" This is incorrect. The constructor you mention there has no effect on whether the `Tracker` copy constructor is automatically created. It is solely because of the deleted copy constructor from the `std::ofstream` member variable. – Nathan Pierson May 03 '22 at 14:21
  • 1
    @NathanPierson - thanks. My mistake. I fixed my answer based on your comment. – wohlstad May 03 '22 at 14:42