Stroustrup's RAII and the cast operator FILE* () = contradiction?

Question

I was reading through Stroustrup’s C++ (3ed, 1997) to see how he implemented the RAII, and on page 365 I found this:

class File_ptr{
    FILE* p;
public:
    File_ptr(const char* n, const char* a){p = fopen(n, a);}
    File_ptr(FILE* pp) { p = pp; }
    ~File_ptr() {fclose(p);}
    operator FILE* () {return p;}
};

The implementation of constructors and destructors is obvious and complies with the RAII idiom, but I don’t understand why he uses the operator FILE* () {return p;}.

This would result in using File_ptr in the following way:

FILE* p = File_ptr("myfile.txt", "r");

The result in a closed p, which is semantically inappropriate in this case. Also, if File_ptr is meant to be used as RAII, this operator allows it to be misused like in the example. Or am I missing something?

Answer 1

Seems like it is inevitable evil price for comfort. Once you want a way FILE* be extractible from your fancy RAII class, it can be misused. Will it be operator FILE*() or FILE* getRawPtr() method, or whatever, it can be called on a temporarty object, making the result invalid just after it is returned.

In C++11, however, you can make this a little more secured, by disallowing this call on temporaries, like this:

operator FILE* () & { return p; }
// Note this -----^

Useful link on how it works provided by Morwenn in the comments: What is "rvalue reference for *this"?

Answer 2

Thinking has moved on quite a way from 1997 as a result of experience, and one of the major recommendations now is not to have implicit cast operators because of problems like this. It was previously thought better to have an implicit conversion operator to make it easier to retrofit to existing code, but this led to problems when the function destroys the resource, as the RAII wrapper class doesn't know about it.

The modern convention is to provide access to the underlying pointer but to give it a name so that at least it's explicit. It won't catch every possible problem but makes it easier to grep for potential violations. For instance with std::string it's c_str():

std::string myString("hello");
callFunctionTakingACharPointer(myString.c_str());
// however...
delete myString.c_str();  // there's no way of preventing this

Answer 3

I don’t understand why he uses the operator FILE* () {return p;}.

The reason for the operator is to provide access/compatibility for APIs that use a FILE*. The problem with the implementation is that it allows client code similar to what you gave as an example.

This would result in using File_ptr in the following way:

FILE* p = File_ptr("myfile.txt", "r");

No actually. While you can do this, the class definition doesn't result in code like this. It is up to you to avoid writing this kind of code (just as it is normally up to you to write code that avoids life-time management issues).

The RAII example in your question is an example of poor design. The conversion operator could be avoided.

I would replace it with a FILE *const File_ptr::get() const accessor. That change doesn't eliminate the problem, but it makes it easier to see in client code that you are returning a const pointer (i.e. "ClientCode, please don't delete this"), from a class.

Answer 4

that's not following the rule of 3 (let alone 5),

so declaring a function as Bar* createBarFromFile(File_ptr ptr) will do unexpected things (the file will be closed after calling this function)

it needs to define a copy constructor and a copy assignment constructor. for rule of 5 it also needs the move variants

however if I'm forced to use a FILE* as member fields I prefer to use a std::unique_ptr<FILE, int (__cdecl *)(FILE *)> and pass &fclose in the constructor