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

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.

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);
  }

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_;
    }
};