1

What is wrong in this code and how to fix it?

int _tmain(int argc, _TCHAR* argv[]) { std::ostream * o = &std::cout; char text[4096]; char *file = "D://test.txt"; if ( file != NULL ) { strcpy ( text, file ); strcat ( text, ".log" ); o = & std::ofstream ( text ); } *o << "test"; //Exception return 0; } 
1
  • Personally I would try and refactor to use references. Commented Aug 5, 2011 at 17:43

6 Answers 6

4
o = & std::ofstream ( text ); 

The right-side expression creates a temporary and you get the address of the temporary which is destroyed at the end of the expression. After that using o would invoked undefined behaviour.

You should be doing this:

{ //... o = new std::ofstream ( text ); if ( *o ) throw std::exception("couldn't open the file"); } //... if ( o != &std::cout ) delete o; //must do this! 
Sign up to request clarification or add additional context in comments.

1 Comment

When I had this problem in the past I used smart pointers, but same concept.
2

It shouldn't compile; the expression std::ofstream( text ) is an rvalue (a temporary), and C++ doesn't allow you to take the address (operator &) of a temporary. And the lifetime of a temporary is only until the end of the full expression, so its destructor will be called (and the memory it resides in may be used for other things) as soon as you pass the ; at the end of the statement.

Just making the ofstream a named local variable doesn't help, either, since the lifetime of a variable is only to the end of the block in which it was declared (the next }). You have to define the std::ofstream before the if, and open it and set o in the if, e.g.:

std::ofstream mayOrMayNotBeUsed; if ( file != NULL ) { // ... mayOrMayNotBeUsed.open( text ); if ( !mayOrMayNotBeUsed.is_open() ) { // Do something intelligent here... } o = &mayOrMayNotBeUsed; } 

5 Comments

It shouldn't compile. Why? I think it will compile, James.
Because it's not legal C++. It has never been legal, neither in C nor in C++, to take the address of a temporary. (§5.3.1/2: "The operand [of unary &] shall be an lvalue or a qualified-id."; (The qualified-id is for taking the address of members.)
Visual Studio allows you to take addresses of temporaries, but I believe it issues a warning.
@Mooing Duck I don't think it even gives a warning, but that might depend on the compiler options. G++ also allows it (for reasons of bug compatibility with VC++), but definitely gives a warning.
@James now that I think on it, I think it only issues warnings if you increase the warning level, but I usually do that on debug builds.
1

o = & std::ofstream ( text ); this creates a temporary ofstream object whose address is assigned to o but the object is instantly destroyed, so o points to a deleted object. This should work (using static):

int _tmain(int argc, _TCHAR* argv[]) { std::ostream * o = &std::cout; char text[4096]; char *file = "D://test.txt"; if ( file != NULL ) { strcpy ( text, file ); strcat ( text, ".log" ); static std::ofstream myofstream( text ); o = &myofstream; } *o << "test"; //Exception return 0; } 

2 Comments

If you like static variables. I wouldn't use it in a multi-threaded context, for example.
I would disagree with the use of static local, and would go with a smart pointer to a dynamically allocated ofstream.
1

The problem is that this code results in undefined behavior:

o = & std::ofstream ( text ); 

When you write

std::ofstream ( text ) 

This creates a temporary ostream object whose lifetime ends as soon as the statement it's in finishes executing. When you take its address and assign it to the pointer o, the pointer now points at a temporary object whose lifetime is about to end. As soon as the statement finishes executing, o is now pointing at an object whose lifetime has ended, so using that object has undefined behavior. Consequently, when you write

*o << "test"; 

You're trying to perform an operation on a dead object, causing problems.

To fix this, you should either

  1. Dynamically-allocate the ofstream by writing o = new std::ofstream(text);, which creates the object such that its lifetime extends past the end of the statement, or
  2. Declare the std::ofstream at the top of _tmain so that its lifetime extends throughout the rest of the function.

Hope this helps!

Comments

1

This

o = & std::ofstream ( text ); 

creates temp object, o starts pointing to the address of this object and later(right after the execution of this row) the object is destroied. Thus undefined behavior (when dereferencing invalid pointer).

The solution - create it with new:

o = new std::ofstraem( text ); 

BUT don't forget to free the allocated memory, before return:

*o << "test"; if ( &std::cout != o ) // don't forget the check .. as I did at the first time { o->close(); // not absolutely necessary, // as the desctructor will close the file delete o; } return 0; 

Comments

0

You are mixing C and C++ in a very unhealthy way, I fear.

First, I heartily recommend using std::string instead of a char*, believe me, you'll have far less troubles.

Second, you should beware of pointers: they may point, if you are not careful, to places in memory that no longer host any "live" object.

I would propose the following code:

void execute(std::ostream& out) { out << "test\n"; } // execute int main(int argc, char* argv[]) { if (argc == 1) { execute(std::cout); return 0; } std::string filename = argv[1]; filename += ".log"; std::ofstream file(filename.c_str()); execute(file); return 0; } 

Which illustrate how to avoid the two pitfalls you fell into:

  • using std::string I avoid allocating a statically sized buffer, and thus I do risk a buffer overflow. Furthermore operations are so much easier.
  • using a function to hoist out the printing logic, I do away with the pointer and the subtle issues it introduced.

It is unfortunate that std::string and std::fstream (and consorts) do not mix so well, at the moment. Historical defect... fixed in C++0x if I remember correctly.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.