4

Consider the following C code:

void openFile(const char *mode, char *filename, FILE **fileptr)
{
  ...
  *fileptr = fopen(filename, mode);
  ...
}

FILE *logstream;
if (LOG_FILE_ENABLED)
{
  openFile("w", "mylogfile.txt", logstream);
}
else
{
  logstream = stderr;
}

fprintf(logstream, "[DEBUG] Some debug message...\n");
fclose(logstream);

I am attempting to translate this to idiomatic C++. How can I overload openFile() such that it takes a std::ofstream, but keep logstream stream-agnostic? I was assuming it would be something like this:

void openFile(const char *mode, char *filename, std::ofstream &ofs)
{
  ...
  ofs.open(filename);
  ...
}

std::ostream logstream;
if (LOG_FILE_ENABLED)
{
  logstream = std::ofstream();
  openFile("w", "mylogfile.txt", logstream);
}
else
{
  logstream = std::cerr;
}

logstream << "[DEBUG] Some debug message..." << std::endl;
logstream.close();

However this is apparently wildly incorrect - you can't even initialize a plain std::ostream like that. How should I handle this - preferably while avoiding the use of raw pointers?

matoro
  • 181
  • 1
  • 13
  • You do not show how `FILE *logstream` is closed, if ever, and how that's handled. – KamilCuk Jan 28 '21 at 17:33
  • Reason for failure: A) Streams cannot be copied. Think of the fun you could have with two copies of the same stream writing to the same underlying medium at the same time. B) [Object Slicing](https://stackoverflow.com/questions/274626/what-is-object-slicing). Workaround: Don't have one. Need to see more context to see if any of the ideas floating around in the back of my head will actually apply. – user4581301 Jan 28 '21 at 17:33
  • `void openFile(const char *mode, char *filename, FILE *fileptr)` wont work very well. I recommend you return the `FILE` pointer instead. – Some programmer dude Jan 28 '21 at 17:42
  • Oops, that was meant to be a double-pointer. You get the idea though. It would be closed with a `fclose(logsream)` – matoro Jan 28 '21 at 18:34
  • `IOStreams` is notorious for its poor design and usability. Use `printf` style format strings or `std::format`, forget `IOStreams` for formatting in modern code, IMO. – Maxim Egorushkin Jan 28 '21 at 20:59
  • Don't unconditionally close the streams. If you use `std::cerr` (or why not `std::clog`?) or `stderr` then you should not close those yourself. – Some programmer dude Jan 29 '21 at 05:16
  • The issue is that it does need to be closed at some point, because in the C original, the same FILE pointer opens and closes several different log files at various times, and points to stderr at other times. I am still not sure how I can directly translate that, since there's no way to call `.close()` on a `std::unique_ptr` – matoro Jan 29 '21 at 15:16

3 Answers3

2

I would move the actual work to a separate function or lambda that takes a std::ostream as input. The caller can then decide which type of std::ostream to pass in, eg:

void doRealWork(std::ostream &log)
{
    ...
    log << "[DEBUG] Some debug message..." << std::endl;
    ...
}

if (LOG_FILE_ENABLED)
{
  std::ofstream log("mylogfile.txt");
  doRealWork(log);
}
else
{
  doRealWork(std::cerr);
}

Or:

auto theRealWork = [&](std::ostream &&log)
{
    ...
    log << "[DEBUG] Some debug message..." << std::endl;
    ...
}

if (LOG_FILE_ENABLED) {
  theRealWork(std::ofstream{"mylogfile.txt"});
} else {
  theRealWork(static_cast<std::ostream&&>(std::cerr));
}

UPDATE: Otherwise, you can do something more like this instead:

using unique_ostream_ptr = std::unique_ptr<std::ostream, void(*)(std::ostream*)>;

unique_ostream_ptr logstream;
if (LOG_FILE_ENABLED) {
  logstream = unique_ostream_ptr(new std::ofstream("mylogfile.txt"), [](std::ostream *strm){ delete strm; });
} else {
  logstream = unique_ostream_ptr(&std::cerr, [](std::ostream *){});
}

*logstream << "[DEBUG] Some debug message...\n";

Or:

using shared_ostream_ptr = std::shared_ptr<std::ostream>;

shared_ostream_ptr logstream;
if (LOG_FILE_ENABLED) {
  logstream = std::make_shared<std::ofstream>("mylogfile.txt");
} else {
  logstream = shared_ostream_ptr(&std::cerr, [](std::ostream*){});
}

*logstream << "[DEBUG] Some debug message...\n";
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • This definitely will not scale - there's thousands of `fprintf()` calls, I can't put them all in an if statement. Also, the log variable is defined externally, before `LOG_FILE_ENABLED` gets set. I need a way such that the logging calls are identical regardless of whether being logged to stderr or a file. – matoro Jan 28 '21 at 18:47
  • Dynamic allocation here is way overkill. – Asteroids With Wings Jan 28 '21 at 19:35
  • @AsteroidsWithWings Perhaps, but I see no other way to do it, given the OP's design restrictions. – Remy Lebeau Jan 28 '21 at 19:36
1

C++ stream library has pretty ancient design. Nevertheless - its basic idea is that ostream or istream are just wrapper objects over stream-buffers.

So you might try something like in this code:

std::ostream get_log(bool str) {
    if (str) return std::ostream(new std::stringbuf());
     // else
    std::filebuf* f = new std::filebuf();
    f->open("log", std::ios_base::out);
    return std::ostream(f);
}

But, as I mentioned, this is very ancient design - so no RAII - this buffer is not owned by stream - you would need to delete it by yourself:

int main() {
    std::ostream log = get_log(true);
    log << "aaa";
    std::cout << static_cast<std::stringbuf&>(*log.rdbuf()).str();
    
    delete log.rdbuf(); // (!)
}

So this is not very usable.

So my final advice - use smart pointer over ostream - like this:

std::unique_ptr<std::ostream> get_log(bool str) {
    if (str) return new std::ostringstream();
    std::ofstream* f = new std::ofstream();
    f->open("log", std::ios_base::out);
    return f;
}

int main() {
    auto log = get_log(true);
    *log << "aaa";
}
PiotrNycz
  • 23,099
  • 7
  • 66
  • 112
1

You were nearly there; you just need to be mindful of scoping rules, and of the fact that there is no such thing as an std::ostream other than as an abstract base class.

So:

std::ostream* logstreamPtr = nullptr;
std::ofstream ofs;
if (LOG_FILE_ENABLED)
{
  logstreamPtr = &ofs;
  openFile("w", "mylogfile.txt", ofs);
}
else
{
  logstreamPtr = &std::cerr;
}

std::ostream& logstream = *logstreamPtr;

logstream << "[DEBUG] Some debug message..." << std::endl;
logstream.close();

You don't need the reference logstream, but it saves you from having to repeatedly reference logstreamPtr later, which would be boring.

Don't be afraid of this raw pointer. This is the purest application of pointers there is. You can go down the smart pointer route if you like, but you gain nothing and lose readability (and, in some cases, performance).

By the way, if you're worried about performance, don't open and close the log file for every single message; that's extremely wasteful.

Asteroids With Wings
  • 17,071
  • 2
  • 21
  • 35
  • `logstream.close()` does not work: `std::ostream has no member 'close'`. Everything else seems to work though. – matoro Jan 28 '21 at 20:53