0

Background:

I am working my way though Stroustrup's c++ book. I am on chapter 10 completing the drills and come across a problem. Now, as far as I can tell I am not strictly following the book, based on how I have seen others answer the questions. I guess Im just after a little more understanding on how to implement the code.

My general aim is to have a write function rather than have it in main().

The aim of the following function is to return an ofstream. 

ofstream& c_data_file()
{
    string oname = "mydata.txt";
    ofstream ost {oname};
    if (!ost) {
        string error = "cannot open " + oname;
        throw runtime_error(error);
    }
    return ost;
}

This code does come with a warning from the compiler and I am including it, as it may be related to the problem. The warning is:

Reference to stack memory associated with local variable 'ost' returned

ostream& operator<<(ostream& os, const Point& p)
{
    return os << "(" << p.x <<"," <<p.y <<")";
}

ofstream& operator<<(ofstream& ost, const Point& p)
{
    return ost << "(" << p.x <<"," <<p.y <<")";
}

the ostream works fine (i use this stream else where). The ofstream does not work, returning the error:

Non-const lvalue reference to type 'ofstream' (aka 'basic_ofstream') cannot bind to a value of unrelated type 'basic_ostream >'

The following is the rest of the relevant code:

int main() {

    vector<Point> original_points;
 ofstream& ost = c_data_file();
    for (int i = 0; i<original_points.size(); i++) {
        ost << original_points[i] << "\n";
    }
    ost.close();
}

I have been looking online all day, the following question seems the closet:

overloaded operator << on ofstream concatenation problems

However, i find it hard to understand the answer and from what i do understand it does not solve my issue.

Thank you for reading and for any help you can give.

I am including the following edit here as well as my response to iksemyonov with the hope to clearly express the problem for everyone. Edit1: The warning was resolved following advice from TBBle. The main issue still stands, as the link indicates and iksemyonov suggests I should use ostream instead of ofstream. This is problematic. Ostream is (following the book) used to send data to cout which is what i am doing (code not present). I also want to send the same data to a file (ideally as the book suggests via ofstream).

Edit 2: following the link and iksemyonov's suggestions worked. A number of the comments below helped me execute the suggestions. So thank you to everyone who commented.

Community
  • 1
  • 1
  • The first warning is because your'e indeed returning a reference to temporary `ost`, which is created on the stack. That can result in a crash since once `ost` goes out of scope after the function returns, it's *not guaranteed* to exist, as it *may* sooner or later be deleted. Thus the warning. – iksemyonov Feb 04 '16 at 04:18
  • 1
    You're returning a reference to a local variable. That is **undefined behavior** – PaulMcKenzie Feb 04 '16 at 04:20

3 Answers3

1

You have created an local ofstream in your function, so you can't return a reference to it, as it's gone after your function ends.

So you have to return an ofstream object instead.

You can't copy streams, so it'll have to be a move. Luckily, that'll be automatic in this case. (Assuming C++11. If not, you need to restructure your code).

And if you're on gcc before gcc 5, you can't move streams due to a bug.

So the simplest change to your function is:

ofstream c_data_file()
{
    string oname = "mydata.txt";
    ofstream ost {oname};
    if (!ost) {
        string error = "cannot open " + oname;
        throw runtime_error(error);
    }
    return ost;
}

and the caller becomes

ofstream ost = c_data_file();
Community
  • 1
  • 1
TBBle
  • 1,436
  • 10
  • 27
  • TBBle, the error seems to be indeed the one described by OP in his link. Namely, that `os << "("` returns an `ostream`, not an `ofstream`. Could you explain the details why that happens? That's the line where the error occurs, checked with a compiler. – iksemyonov Feb 04 '16 at 04:36
  • Actually, I'd overlooked that this was the error, I was looking at the warning. Your answer above addresses the actual issue. – TBBle Feb 04 '16 at 04:50
  • Thank you TBBle, this fixed the warning. But has not resolved the main issue. –  Feb 04 '16 at 05:03
1

Based on the link provided by OP,

overloaded operator << on ofstream concatenation problems

other than fixing the returning reference to temporary error, the fix is to change

ofstream& operator<<(ofstream& ost, const Point& p)
{
    return ost << "(" << p.x <<"," <<p.y <<")";
}

to

ostream& operator<<(ostream& ost, const Point& p)
{
    return ost << "(" << p.x <<"," <<p.y <<")";
}

which compiles just fine. As the answer in the link describes, the reason for the error to occur is that ost << "(" returns an object of type ostream, and the further calls to operator<< thus fail because of the incompatibility reported by the compiler.

On a side note, it is not a good style to not return an exit code from main, as in return 0.

Community
  • 1
  • 1
iksemyonov
  • 4,106
  • 1
  • 22
  • 42
  • Ok, my problem with this is that I am already using ostream to send data to cout. Can i also use it at the same time to send data to a file (or ost)? I just tried removing ofstream and keeping only the ostream and that created a linker error and another error. –  Feb 04 '16 at 05:13
  • What's the "another error"? This answer is correct. An `ofstream` is an `ostream`, so providing just one `ofstream& operator<<(ofstream&, const Point&)` should fulfil both use-cases. – TBBle Feb 04 '16 at 05:42
  • Ok its working now. Simply by removing ofstream, but properly this time. I forgot the linker in the header.h file, I feel like an idiot. –  Feb 04 '16 at 06:43
  • Not returning an exit code is not incorrect; the compiler is required to behave as if you ended `main` with `return 0;`. – user253751 Feb 04 '16 at 20:02
  • fixed to be "bad style" which was what I actually meant. – iksemyonov Feb 04 '16 at 20:59
0

ofstream& c_data_file() is returning a reference (&) to an ofstream ost; which no longer exists (ofstream ost; is a local object and is gonna be destroyed once it goes out of scope)

so what you could do instead is modify your function to ofstream c_data_file() and this way you will no longer return a temporary object.

Jts
  • 3,447
  • 1
  • 11
  • 14