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?

I think the point of that operator is so that you can pass a `File_ptr` to functions such as `fputs` that accept a `FILE *`, not a `File_ptr`. Yes, this definition allows it to be used incorrectly. — , Feb 27 '14 at 12:20
That fact that the file is closed in your example is exactly the point of RAII. File_ptr is responsible for this resource over its entire lifetime. — Till, Feb 27 '14 at 12:24
You're correct. This is one of the reasons `std::string` doesn't have an `operator char const*()`. — Simple, Feb 27 '14 at 12:24
You should take a look at `unique_ptr`, which is a modern (C++ 11) class with similar semantics. It uses a method called `get()` in this context. — CodesInChaos, Feb 27 '14 at 13:35
Its not about the operator or a member function. You can still write, `const char* val = std::string("blah").c_str()`. — balki, Feb 27 '14 at 15:49
The difference is significant, because with the operator chances are much higher that this happens *accidentally*. — Christian Hackl, Feb 27 '14 at 16:16
Indeed. The fact that this can happen accidentally is exactly what I was alluding to in my question. That's why I find this `&`-thingy so nice. (see below in Mikhail answer.) — Davor Josipovic, Feb 27 '14 at 16:58

score 28 · Accepted Answer · edited May 23 '17 at 12:01

28

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"?

edited May 23 '17 at 12:01

Community

1
1

answered Feb 27 '14 at 12:30

Mikhail

18,155
5
56
129

This is the first time I see something about disabling an operator for temporaries. Can you please provide me with any reference or search term for finding more details? Thanks. – utnapistim Feb 27 '14 at 12:36
7

@utnapistim That's [rvalue references for `*this`](http://stackoverflow.com/q/8610571/1364752). – Morwenn Feb 27 '14 at 12:37
3

This will disallow usage like `fgetc(File_ptr("my_file"))` Which might be useful – balki Feb 27 '14 at 15:54

score 7 · Answer 2 · answered Feb 27 '14 at 12:25

7

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

answered Feb 27 '14 at 12:25

the_mandrill

27,460
4
58
90

technically it's `data()` but `c_str()` does that same in C++11 – ratchet freak Feb 27 '14 at 23:29
Really? I've been using `c_str()` for years on multiple platforms – the_mandrill Feb 28 '14 at 09:42
yeah but data() is not guaranteed to be null terminated in C++98 so c_str could cause a realloc – ratchet freak Feb 28 '14 at 09:47
@ratchet: Is there an actual implementation that does *not* save the string in null-terminated representation already? It seems like a very reasonable thing to do since the overhead is so low – Niklas B. Mar 02 '14 at 19:02
@NiklasB. if you allow the array to be shared to quicker substring allocation but that create unpredictable timings when asking the c_str and/or manipulate it – ratchet freak Mar 02 '14 at 20:45
@ratchetfreak: But as I understand it sharing is also something that is not often done in practice, because it tends to create memory leakes. – Niklas B. Mar 02 '14 at 20:47

score 4 · Answer 3 · answered Feb 27 '14 at 12:34

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.

score 4 · Answer 4 · answered Feb 27 '14 at 14:17

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

answered Feb 27 '14 at 14:17

ratchet freak

44,814
5
55
99

+1 for std::unique_ptr. I've been doing this myself, but I wasn't sure if it was a practice. – Jesse Hallam Feb 27 '14 at 23:23
1

@Kivin another way is to create a stateless FileDeleter class with a `void operator()(FILE* f){fclose(f);]` and use that as the second template parameter, upside of that is that you can typedef it without needing to fiddle with the constructor – ratchet freak Feb 27 '14 at 23:27

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

4 Answers4