Accept either a existing stream or a filename (to be opened) in a constructor

Question

Context:

In a larger project, I am trying to build an utility class to encapsulate the parsing of input data. And I want it to be able to process either an already existing input stream, or a file given by its name.

The general idea is to build a stream converter. The input file contains a variable number of (possibly multi-line records). The program parses the file, retrieves one item, convert it and write it to the output file. So the structure of the program (pseudo code here) is:

Parser parser(input_stream);
Writer writer(output_stream);
while (StopRecord != (record = parser.getRecord())) {
 writer.write(record);
}

My first idea was to have a ref to a std::istream as a class member, and initialize it in constructor, either from an existing ref or to a newly opened file

class Parser {
 std::istream ∈
public:
 Parser(std::istream& in): in(in) {} // fine
 Parser(std::string file): in(std::ifstream(file)) {} // compile error
 Record getRecord();
};

The problem is that std::ifstream(file) is a temporary, and can only initialize a const lvalue reference or a rvalue reference. A const istream would be pretty useless, so I tried a rvalue ref.

class Parser {
 std::istream &∈
public:
 Parser(std::istream& in): in(in) {} // compile error
 Parser(std::string file): in(std::ifstream(file)) {} // fine
 Record getRecord();
};

But now a std::istream& cannot be used to initialize a std::istream&&, and I would not dare to use Parser(std::istream&& in): in(in) {} and later p = Parser(std::cin); with a move from a standard stream!

Current code:

I finally decided to dynamically allocate a std::ifstream in my class when I have to process a file to have an lvalue to initialize my ref:

class Parser {
 std::ifstream *fin;
 std::istream& in; // MUST be after fin declaration!
public:
 Parser(std::istream& in): fin(nullptr), in(in) {}
 Parser(std::string file): fin(new std::ifstream(file)), in(*fin) {}
 ~Parser() {
 if (fin) {
 fin.close();
 delete fin;
 }
 }
 Record getRecord();
};

It seems to work (even if in case of copies all will share the same underlying stream), but requires fin to be declared before in, and the raw pointer initialization looks rather ugly.

Question:

As this design come from my former C experience, I wonder whether this follows modern C++ best practices and how I could make it better, more robust and easier to maintain

• Zeta: "(even if in case of copies all will share the same underlying stream)" Undefined behaviour, delete called multiple times on same object.
• Serge Ballesta: @Zeta: Thank you for noticing! I would better delete the copy and move ctors, or make them explicit and ensure fin exists only in one copy.
• Incomputable: By the way, what parser exactly do you want to build? I mean, what does it do?
• Serge Ballesta: @200_success: sorry to have missed the point on code not yet written. I've tried to add relevant information. Could you tell me what is still lacking here for an acceptable question.
• 200_success: The code doesn't compile. There is at least a problem with mismatched braces.

Answer 1

Very close

You're actually one foot off the finish.

Reduce

You could store a pointer to the istream (note that std::ifstream is its child class, so new std::ifstream(file) will automatically be converted to istream*), then store a bool flag if it is owning or not.

Bigger issue

There is yet greater issue here, and it is ownership semantics. When ownership semantics are weird design starts becoming fragile. I'd recommend letting the caller to take ownership of the stream, so that Parser wouldn't mess with it and keep rule of zero. Callers will have much more power that way. Otherwise Parser would also require implementing either of rule of 0/3/5.

Code review

fin.close();

is redundant in the destructor. Invoking it explicitly opens possibilities for exceptions, thus making your fin leak. Just delete it, destructor will take care of closing.

Code never checks if the file is opened. Code could start on non-opened stream, which is, I believe undefined behavior.

• Serge Ballesta: Well, the bigger issue part leads not far from don't... Which would cause a problem if I wanted to dynamically create a Parser from a file: the caller would have to maintain the lifetime of the stream at least until the destruction of the parser. But the 2 other points will really help.
• Incomputable: @SergeBallesta, the issue lies in design of iostreams. They are quite ancient, and I believe were one of the first things created (e.g. before C++ was even standardized in 1998). I've dealt with it the way I describe above, and it didn't cause any issues. Users should not dynamically allocate streams anyway. std::ifstream file(...); parser p; //use parser would be quite idiomatic and easy to use. Feel free to downvote though, I'd be interested in other suggestions.
• Incomputable: @SergeBallesta, IIRC, they are the most Java-ish part in the whole C++ (automatic deletion, inheritance, etc). I believe studying them more will reveal quite a lot of story behind evolution of C++.

Answer 2

As @incomputable mentions ownership is an issue, by carrying the reference in your parser class you will be relying on the fact that the original caller keeps the stream alive. Or doesn't call close() on it.

Depending on where the stream is coming from std::shared_ptr<std::istream> in the constructor might express better what you are trying to do, but that would not work well with std::cin

Without having the rest of the functionality of the parser it is hard to tell. I would prefer the stream being passed to the parser in the parse() call. The somewhat iffy (IMHO) construct that you have here then just becomes a plain interface.

class Parser {
 bool parse(std:istream& in)
 {
 // Do work
 return error;
 } 
 bool parse(const std::string& filename)
 {
 std::istream in(filename);
 // deal with errors
 return parse(in);
 }

• Serge Ballesta: Well, I initially wanted to be able to parse one item at a time, without first reading everything, but it seems to add a lot of complexity. My be I should rework the whole design.
• Harald Scheirich: @SergeBallesta that kind of question is beyond what code review does ... but it really all depends on the needs. If you post more code you might get a better answer here. But also sometimes less is more if your format is simple do you really need a class.
• Barmar: "that would be work well" should be "that would not work well"?

Answer 3

Here's a suggestion leveraging std::optional.

#include <utility>
#include <experimental/optional>
#include <iostream>
#include <fstream>
#include <string>
namespace std {
class Parser {
 std::optional<std::ifstream> fin { std::nullopt };
 std::istream& in;
public:
 Parser(std::istream& in_): in(in_) {}
 Parser(std::string file): fin(std::in_place_t, file), in(fin.value()) {}
 // No need for ~Parser()! {}
 // parsing members and methods omitted for brievety
};

This is inelegant, in that you "waste" storage on a nullopt you don't need in the non-filename case, but since you're not holding millions of these objects at once it's justifiable more than having some inheritance or templating to avoid it.

Also - this code is C++17, but you can approximate well enough with C++14 and a friendly compiler to elide a temporary.

Answer 4

Here is another possible solution with pointer and flag. I know that it could be not so fancy and modern. But it's just work:)

class Parser {
 std::istream* in_;
 const bool own_;
public:
 Parser(std::istream& in): in_(&in), own_(false) {}
 Parser(const std::string& file): in_(new std::ifstream(file)), own_(true) {}
 // disallow
 Parser(const Parser&) = delete;
 Parser& operator=(const Parser&) = delete;
 Parser(Parser&& other): in_(other.in_), own(other.own_) {
 other.in_ = nullptr; // prevent from deleting
 }
 Parser& operator=(Parser&& other) {
 if (own_) delete in_;
 in_ = other.in_;
 other.in_ = nullptr; // prevent from deleting
 own_ = other.own_;
 }
 ~Parser() {
 if (own_) delete in_;
 }
};