Search code examples
c++fileclassraii

assignment fails for a FILE* class


I tried to wrap class around FILE*, here it is

class file_ptr
{
    public:
        file_ptr(const wstring& _FileN, const wstring& _OpenMode) : file_n(_FileN), omode(_OpenMode),
        fptr(_wfopen(file_n.c_str(), omode.c_str()))
        {
            if (!fptr)
                throw wstring(L"Failed to open File ") + _FileN;
        }

        ~file_ptr()
        {
            fclose(fptr);
        }

        file_ptr& operator =(const file_ptr& other)
        {
            if (this != &other)
            {
                this->~file_ptr();
                fptr = other.fptr;
                file_n = other.file_n; omode = other.omode;
            }
        }

        operator FILE* () { return fptr; }

    private:
        wstring file_n, omode;
        FILE* fptr;
};

why wstring? I need Unicode support.

now the problem lets say it did something like this

int main() {
    try {
        file_ptr file1(L"text1",L"wb");
        fwrite("Hello",1,5,file1);
        file1 = file_ptr(L"text2",L"wb");
        fwrite("Hello",1,5,file1);
    } catch ( const wstring& e ) {
        wcout << e << endl;
    }
    return 0;
}

Nothing will be written in text2

I even tried after removing my assignment overload, becoz I suppose the default behaviour should be same, but the problem persists

it works if I use raw FILE* as expected f.e

int main() {
    try {
        FILE* file1 = _wfopen(L"text1",L"wb");
        fwrite("Hello",1,5,file1);
        fclose(file1);
        file1 = _wfopen(L"text2",L"wb");
        if (!(file1))
            throw L"Can't open file";
        fwrite("Hello",1,5,file1);
    } catch ( const wstring& e ) {
        wcout << e << endl;
    }
    return 0;
}

text2 is written correctly,


Solution

  • file1 = file_ptr(L"text2",L"wb"); expression creates a temp file_ptr object and then fptr = other.fptr; copies a FILE pointer value owned by temp object. Temp object gets destroyed immediately and closes file pointer leaving file1 with a dandling FILE pointer. You should write a move assignment operator instead:

    file_ptr &
    operator =(const file_ptr & other) = delete; // prevent accidental use
    
    file_ptr &
    operator =(file_ptr && other) noexcept
    {
        if(this == ::std::addressof(other))
        {
            ::std::terminate(); // there is no context when selfassignment makes sense
        }
        //this->~file_ptr(); calling destructor on itself is no good
        ::fclose(this->fptr);
        this->fptr = other.fptr;
        other.fptr = 0;
        this->file_n = ::std::move(other.file_n);
        this->omode = ::std::move(other.omode);
        return(*this);
    }
    

    As mentioned in comments, it would be a good idea to disable copy constructor and implement move constructor to prevent similar problems occurring during construction. You may also want to check Rule-of-Three becomes Rule-of-Five with C++11?