Best practice when dealing with C++ iostreams

Question

I'm writing a command-line utility for some text processing. I need a helper function (or two) that does the following:

1. If the filename is -, return standard input/output;
2. Otherwise, create and open a file, check for error, and return it.

And here comes my question: what is the best practice to design/implement such a function? What should it look like?

I first considered the old-school FILE*:

FILE *open_for_read(const char *filename)
{
    if (strcmp(filename, "-") == 0)
    {
        return stdin;
    }
    else
    {
        auto fp = fopen(filename, "r");
        if (fp == NULL)
        {
            throw runtime_error(filename);
        }
        return fp;
    }
}

It works, and it's safe to fclose(stdin) later on (in case one doesn't forget to), but then I would lose access to the stream methods such as std::getline.

So I figure, the modern C++ way would be to use smart pointers with streams. At first, I tried

unique_ptr<istream> open_for_read(const string& filename);

This works for ifstream but not for cin, because you can't delete cin. So I have to supply a custom deleter (that does nothing) for the cin case. But suddenly, it fails to compile, because apparently, when supplied a custom deleter, the unique_ptr becomes a different type.

Eventually, after many tweaks and searches on StackOverflow, this is the best I can come up with:

unique_ptr<istream, void (*)(istream *)> open_for_read(const string &filename)
{
    if (filename == "-")
    {
        return {static_cast<istream *>(&cin), [](istream *) {}};
    }
    else
    {
        unique_ptr<istream, void (*)(istream *)> pifs{new ifstream(filename), [](istream *is)
                                                      {
                                                          delete static_cast<ifstream *>(is);
                                                      }};
        if (!pifs->good())
        {
            throw runtime_error(filename);
        }
        return pifs;
    }
}

It is type-safe and memory-safe (or at least I believe so; do correct me if I'm wrong), but this looks kind of ugly and boilerplate, and above all, it is such a headache to just get it to compile.

Am I doing it wrong and missing something here? There's gotta be a better way.

Answer 1

I would probably make it into

std::istream& open_for_read(std::ifstream& ifs, const std::string& filename) {
    return filename == "-" ? std::cin : (ifs.open(filename), ifs);
}

and then supply an ifstream to the function.

std::ifstream ifs;
auto& is = open_for_read(ifs, the_filename);

// now use `is` everywhere:
if(!is) { /* error */ }

while(std::getline(is, line)) {
    // ...
}

ifs will, if it was opened, be closed when it goes out of scope as usual.

A throwing version might look like this:

std::istream& open_for_read(std::ifstream& ifs, const std::string& filename) {
    if(filename == "-") return std::cin;
    ifs.open(filename);
    if(!ifs) throw std::runtime_error(filename + ": " + std::strerror(errno));
    return ifs;
}