Using smart pointers in a struct or class

Question

I've written a function that loads the bytes off a file and returns a FileData struct that contains the byte buffer and the length of the buffer.

I want the buffer to be deleted as soon as it's consumed and thrown out of scope.

I'm having trouble getting it to compile due to various casting errors. Also, I'm not sure whether the buffer is being moved correctly rather than copied. I don't mind the FileData struct itself being copied, since it's maybe 16 bytes at most.

In general, how do you use smart pointers as class/struct fields? Is that even something you'd do?

This is a bit of a nebulous question, I know, but since I'm having some conceptual difficulties with smart pointers in general, I'm hoping that this example will help me in the right direction.

Here's what I've got so far:

struct FileData
{
    unique_ptr<char[]> buf;
    unsigned int len;
};

FileData LoadFile(string filename)
{
    ifstream str;
    str.open(filename, ios::binary);

    str.seekg(0, ios::end);
    auto len = str.tellg();
    str.seekg(0, ios::beg);

    char* buf = new char[len];

    str.read(buf, len);
    str.close();

    FileData d = { unique_ptr<char[]>(buf), len };

    return d;
}

Edit: Since some people are curious about the error message that I get with this current code, here it is:

error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access private member declared in class 'std::unique_ptr<_Ty>'

Answer 1

Your code is fine, except for one little detail:

struct FileData
{
    unique_ptr<char[]> buf;
    <del>unsigned int</del> <ins>streamoff</ins> len;
};

The reason it doesn't compile for you is that your compiler does not yet implement the automatic generation of the special move members. In a fully C++11 conforming compiler your FileData would behave as if:

struct FileData
{
    unique_ptr<char[]> buf;
    streamoff len;

    FileData(FileData&&) = default;
    FileData& operator=(FileData&&) = default;
    FileData(const FileData&) = delete;
    FileData& operator=(const FileData&) = delete;
    ~FileData() = default;
};

The defaulted move constructor simply move constructs each member (and similarly for the defaulted move assignment).

When returning d from LoadFile, there is an implicit move that takes place which will bind to the implicitly defaulted move constructor.

Use of vector<char> or string as others have suggested will also work. But there is nothing wrong with your code as far as C++11 is concerned.

Oh, I might tweak it like so: I like to get my resources owned as quickly as possible:

FileData LoadFile(string filename)
{
    ifstream str;
    str.open(filename, ios::binary);

    str.seekg(0, ios::end);
    auto len = str.tellg();
    str.seekg(0, ios::beg);

    FileData d = {unique_ptr<char[]>(new char[len]), len};

    str.read(d.buf.get(), d.len);
    str.close();

    return d;
}

If you need to explicitly define the FileData move members, it should look like:

struct FileData
{
    unique_ptr<char[]> buf;
    streamoff len;

    FileData(FileData&& f)
        : buf(std::move(f.buf)),
          len(f.len)
        {
            f.len = 0;
        }

    FileData& operator=(FileData&& f)
    {
        buf = std::move(f.buf);
        len = f.len;
        f.len = 0;
        return *this;
    }
};

Oh, which brings me to another point. The defaulted move members are not exactly correct since they don't set len to 0 in the source. It depends on your documentation if this is a bug or not. ~FileData() doesn't require len to reflect the length of the buffer. But other clients might. If you define a moved-from FileData as not having a reliable len, then the defaulted move members are fine, else they aren't.

Answer 2

I would probably use a std::vector instead of a std:::unique_ptr<char[]>, if you don't mind the std::vector being copied when you return the FileData:

struct FileData 
{ 
    vector<char> buf; 
}; 

FileData LoadFile(string filename) 
{ 
    ifstream str; 
    str.open(filename, ios::binary); 

    str.seekg(0, ios::end); 
    auto len = str.tellg(); 
    str.seekg(0, ios::beg); 

    FileData d; 
    d.buf.resize(len); 

    str.read(&(d.buf)[0], len); 
    str.close(); 

    return d; 
} 

Alternatively, to avoid the copy, the caller can pass in a FileData as a function parameter instead of a return value:

struct FileData 
{ 
    vector<char> buf; 
}; 

void LoadFile(string filename, FileData &data) 
{ 
    ifstream str; 
    str.open(filename, ios::binary); 

    str.seekg(0, ios::end); 
    auto len = str.tellg(); 
    str.seekg(0, ios::beg); 

    data.buf.resize(len); 

    str.read(&(data.buf)[0], len); 
    str.close(); 
}