7
\$\begingroup\$

Problem description

As the title indicates, I want to be able to write to and read from a binary file safely, in the sense that I want to have full control of everything that might go wrong. Since I am somewhat unfamiliar with all the flags of the ofstream class and not sure how to use them, I wrote the following code as an exercise.

Code

#include <filesystem>
#include <algorithm>
#include <iostream>
#include <fstream>
#include <string_view>

using DType = int;
using namespace std;
namespace fs = filesystem;

int main(int argc, char* argv[]) {

  /* Command line arguments */

  if (argc == 1) {
    cout << "Usage: <executable> -f <filename>" << endl;
    exit(-1);
  }
    
  string_view filename {""s};
  for (int i=1; i<argc; i++) {
    if ((argv[i] == "-f"s) && (argv[++i] != nullptr)){
      filename = argv[i];
    }
    else {
      cout << "Provide a filename with -f <filename>." << endl;
      exit(-1);
    }
  }
  
  /* Create file if it does not exist */
  
  fs::path wdir = fs::current_path();
  fs::path filepath = wdir/filename;
  fstream outfile;
  
  if (!fs::exists(filepath)) {
    auto touch = ofstream{filepath};
    cout << "File " << filepath << " created." << endl;
  }
  else {
    cout << "File " << filepath << " exists and cannot be created." << endl; 
    string yesOrNo = {""s};
    bool isFalse = 1;
    do {
      cout << "Do you wish to delete the file? Type yes/no and return." << endl;
      cin >> yesOrNo;
      for_each(yesOrNo.begin(), yesOrNo.end(), [](char& c){c = tolower(c);});
      if (yesOrNo == "yes"s) { 
        fs::remove(filepath);
        cout << "Old file " << filepath << " removed." << endl;
        auto touch = ofstream{filepath};
        cout << "New file " << filepath << " created." << endl;
        isFalse = 0;
      }
      else if (yesOrNo == "no"s) {
        cout << "Program exiting." << endl;
        exit(-1);
      }  
    }while (isFalse);
  }
  
  /* Open file and write to it */
  
  DType bufToWrite[10] = {0,1,2,3,4,5,6,7,8,9};
  
  outfile.open(filepath, ios::in | ios::out | ios::binary);
  if (!outfile) {
    throw invalid_argument{"Cannot open file: "s + filepath.string()};
  }
  
  outfile.write(reinterpret_cast<char*>(&bufToWrite), sizeof(bufToWrite));
  if(outfile.good()) {
    cout << "Writing to file " << filepath << " succeeded." << endl;
    outfile.close();
  }
  else {
    cout << "Writing to file " << filepath << " failed." << endl;
    exit(-1);
  }
  
  /* Read the created file and store its content */
  
  size_t bufSize = sizeof(bufToWrite)/sizeof(DType);
  DType toRestore[bufSize] = {};
  ifstream infile(filepath, ios::binary);
  if(infile){
    infile.read(reinterpret_cast<char*>(&toRestore), sizeof(toRestore)); 
    if(infile.good()) {
      cout << "Reading from file " << filepath << " succeeded." << endl;
      infile.close();
    }
    else {
      cout << "Reading from file " << filepath << " failed." << endl;
      exit(-1);
    }
  }
  else{
    cout << "File " << filepath << " does not exist." << endl;
    exit(-1);
  }
  
  /* Final print out */
  
  cout << "Printing contents of " << filepath << " file:\n";
  for (DType elem : toRestore) cout << elem << "\t";
  cout << "\nProgram terminated sucessfully. " << endl;
  return 0;
}

What are the pitfalls that one should avoid and how to do so? For example when should I use good() and avoid the sticky bits? For example here https://stackoverflow.com/questions/28342660/error-handling-in-stdofstream-while-writing-data it says that

In principle, if there is a write error, badbit should be set. The error will only be set when the stream actually tries to write, however, so because of buffering, it may be set on a later write than when the error occurs, or even after close. And the bit is “sticky”, so once set, it will stay set.

I understand my question is general, but If you can mention all the spots that can go wrong and how one should properly deal with them in C++17, I would be grateful. General notes concerning style etc. are most welcome.

\$\endgroup\$
4
\$\begingroup\$

Sorry it took so long—this is a very busy time of year for me (and it’s getting worse)—but as promised, here is an error-handling-focused review. This review is laser-focused just on the error-handling in the original code; other reviews do a fine job of covering other angles. The main focus is going to be error-handling with IOstreams, but I’ll brush on some general error-handling issues as well.

So here we go.

General error-handling issues

I wish I could give you better news, but error-handling in C++ is a goddamn mess. Even just within the standard library, there are at least a half-dozen different error-handling paradigms in use:

  • Return status codes, e.g. printf().
  • Special error values embedded in return. This is different from the previous in that if you are not interested in error handling, you can simple ignore the return value of printf()… but in this case, the return value is actually important—it’s what you want—but special values indicate failue. e.g. malloc() (0 indicates failure).
  • Status codes as part of returned structures, e.g. from_chars().
  • Global status variables, e.g. errno. This also includes sorta-global status variables, like thread local or function static variables (like strtok()).
  • “Out” parameters, either passed by pointer or reference, e.g. the <filesystem> functions that take error_code& args.
  • Exceptions, obviously. Numerous examples.
  • <system_error>, e.g. the <filesystem> functions that don’t take error_code& args.
  • … and probably more.

Error handling is such a raging dumpster fire in C++, that there are numerous new solutions being developed—this is a hot topic of research. The most interesting are perhaps:

The big hold-up on these things is that there are some really incredible features coming in C++23 and beyond that would be game-changers for implementing these, so they’re on hold until those other features get implemented. As just one example: pattern matching. If we get pattern matching, it would be nice if error-handling integrated well with it.

And for all that bad news, it gets even worse: IOstreams is the second-worst part of the standard library, when it comes to error-handling. (The dishonour of being the worst part of the standard library, when it comes to error-handling, is undoubtedly <filesystem>.)

I know that makes IOstreams sound shitty, and I admit I frequently dump on it… very frequently… but the truth is, IOstreams is pretty incredible. I noticed @Peter-ReinstateMonica doing a survey about whether one would use IOstreams for your program’s purpose, or the C stdio library, or platform-specific calls, and it struck me as an amusing question, because there is only one really correct answer: IOstreams. I can’t even fathom why someone would purposefully choose to use stdio when they have other options; stdio is just irreparably broken in so many ways (it’s utterly impossible to use safely in multi-threaded code, for example). And platform-specific calls are… well… platform-specific; by contrast, with IOstreams, the code can theoretically work on any platform, and can transparently work with any kind of I/O: in-memory I/O, file I/O, network I/O, and so on. Unless you were just slapping a quickie program together—in which case, sure, maybe using platform-specific I/O will be simple and efficient—there really is no other sensible choice (well, maybe there are third-party C++ I/O libraries one might consider).

So IOstreams is not perfect… it’s got piles of problems… but it is very, very good. And it should be your default choice for all I/O in C++.

Nevertheless, by the standards of high-quality, modern C++, some of what follows is going to be… inelegant. Just keep that in mind.

You must catch exceptions

Exceptions were added very late in the original C++ standardization process, and at the time were poorly understood… it wasn’t until a couple years later that Dave Abrahams formalized exception safety. People were terrified of exceptions at first, and some still are. There is a lot of misinformation out there about exceptions.

Because of that, there is a very dangerous hole in the specification of exceptions in C++.

If exceptions are thrown and caught normally, there are no problems; everything works predictably. (Exceptions are non-deterministic in terms of complexity—that is, how long they’ll take—but are deterministic in terms of behaviour… you don’t necessarily know how long it will take for an exception to be completely handled, but you know for sure exactly what will happen when it does, including everything that happens along the way.)

The problem is what happens if an exception is thrown but never caught.

To illustrate the problem (source here):

#include <iostream>

struct foo
{
    foo() { std::cout << "construct\n"; }
    ~foo() { std::cout << "destruct\n"; }
};

auto main() -> int
{
    auto f = foo{};

    throw 1;
}

When run, may produce:

construct

See the problem? The foo object is constructed… but never destroyed.

When an exception is thrown but never caught, std::terminate() is called, and… and this is the important part… the stack is not (necessarily) unwound.

When the stack is not unwound, destructors are not called. This is bad… this is very bad. Arguably, the single most important feature of C++—the THING that separates it from other languages—is its deterministic destructors. I’ve seen it claimed that the most important line of code in C++ is “}”, because that is what triggers destructors. Destructors release and clean up resources, which means that if they’re not run, you will leak those resources, and a lot of clueless people will shrug and say, “so what; if the program is ending anyway, it’s fine to leak resources, because the OS will clean them all up anyway.” That “thinking” is relatively harmless for things like memory, which, yes, if your program ends while still holding memory, the OS will reclaim it… but memory is not the only resource C++ classes manage. Destructors are also used to commit or roll back database transactions, they’re used to send shut down sequences to external hardware, and so much more that is outside of the scope of the OS. If you fail to release these resources, you can lock up external hardware, you can leave online databases in indeterminate state, and who knows what else.

So failing to run destructors is very, very bad, and you must do everything in your power to ensure that destructors are run, even when your program is crashing.

(Obviously there are exceptional cases where priorities are different. For example, if your code is control software for a self-driving car, you want it to crash FAST, and restart just as fast. But that is not the general case, obviously.)

So, whether you explicitly throw exceptions (as you do with throw invalid_argument{"Cannot open file: "s + filepath.string()};) or not (because you must always be exception safe), you should defend against them going uncaught. You can’t do much about exceptions thrown from static objects… but you shouldn’t really be doing anything complicated or dangerous in static objects anyway. But you can do something to prevent exceptions escaping main(). You can do something like this:

auto main() -> int
{
    try
    {
        // your whole program goes here
    }
    catch (...)
    {
    }
}

This is the bare minimum you should do.

To improve this, you could return EXIT_FAILURE in the catch block. (Note: NOT -1. -1 is not portable.)

#include <cstdlib>

auto main() -> int
{
    try
    {
        // your whole program goes here
    }
    catch (...)
    {
        return EXIT_FAILURE;
    }
}

Personally, I use what’s called a Lippincott function to print some information about the fact that there was an unhandled exception:

// in a separate module/translation unit -----------------------

// Note: Once written, this module can be copy-pasted into new
// projects, and used without modification.

namespace indi {

auto handle_uncaught_exception() -> int
{
    try
    {
        throw;
    }
    // you can catch custom exception types if you like
    //catch (indi::exception const& x)
    //{
    //}
    catch (std::exception const& x)
    {
        std::cerr << "unhandled exception: " << x.what() << '\n';

        // technically, writing to std::cerr, and some of this other
        // stuff, might throw... in which case, the exception will
        // escape, and std::terminate() will be called.
        //
        // but by this point, all of the non-static destructors will
        // already have been run, so this situation, while not *great*
        // is not all that serious.
    }
    catch (...)
    {
        std::cerr << "unhandled exception: "
            << "<unknown exception type>"
            << '\n';

        // perhaps:
        //std::cerr << boost::current_exception_diagnostic_information();
    }

    return EXIT_FAILURE;
}

} // namespace indi

// in the main source file -------------------------------------

auto main() -> int
{
    try
    {
        // your whole program goes here
    }
    catch (...)
    {
        return indi::handle_uncaught_exception();
    }
}

You should consider the above boilerplate to be standard code that you have to include for EVERY non-trivial project.

Never use std::exit()

You use std::exit() numerous times in your code, often calling it as exit(-1) to exit immediately with a failure status.

There are two problems here:

  1. -1 is not a portable exit status. There are only 3 portable exit statuses: 0 and EXIT_SUCCESS to indicate success (EXIT_SUCCESS may equal zero, or it may not), and EXIT_FAILURE to indicate failure. So you should be calling exit(EXIT_FAILURE). Except…
  2. std::exit() is a C library function. You should never, ever use it in a C++ program.

What’s the problem with std::exit()? Once again, it’s about destructors:

#include <cstdlib>
#include <iostream>

struct foo
{
    foo() { std::cout << "construct\n"; }
    ~foo() { std::cout << "destruct\n"; }
};

auto main() -> int
{
    auto f = foo{};

    std::exit(0);
}

When run, may produce:

construct

std::exit() is slightly less dangerous than leaving an exception uncaught… but still very dangerous.

So, now you have a program that calls std::exit() everywhere. How do you fix it?

Well, as a transitional step, you could do something like this:

// in a reusable module/translation unit -----------------------

namespace indi {

struct clean_exit_t
{
    int status = 0;
};

[[noreturn]] auto clean_exit(int status) -> void
{
    throw clean_exit_t{status};
}

[[noreturn]] auto clean_exit() -> void
{
    clean_exit(0);
}

[[noreturn]] auto clean_fail() -> void
{
    clean_exit(EXIT_FAILURE);
}

auto handle_uncaught_exception() -> int
{
    try
    {
        throw;
    }
    catch (clean_exit_t x)
    {
        return x.status;
    }
    // ... and so on ...
    catch (...)
    {
        std::cerr << "unhandled exception:\n"
            << boost::current_exception_diagnostic_information();
    }

    return EXIT_FAILURE;
}

} // namespace indi

And everywhere your program does:

if (/*whatever*/)
{
    cout << "..." << endl;
    exit(-1);
}

Instead do:

if (/*whatever*/)
{
    std::cerr << "...\n";
    indi::clean_fail();
}

But this is just a temporary fix (actually, a bit of a hack, really). The proper solution is to use to purpose-specific exceptions, and to catch them, and report the error properly. For example, this is what just the first bit of your program might look like:

auto main(int argc, char* argv[]) -> int
{
    try
    {
        try
        {
            if (argc == 1)
                throw no_filename_argument_provided{};

            // ... and so on ...
        }
        catch (no_filename_argument_provided const&)
        {
            std::cerr << "Usage: <executable> -f <filename>\n";

            return EXIT_FAILURE;
        }
    }
    catch (...)
    {
        return indi::handle_uncaught_exception();
    }
}

You may find the nested try blocks ugly, and I don’t disagree. You could integrate the program-specific error handling in the handle_uncaught_exception() function… but then that function is no longer reusable for different programs. You could also try something “clever” like using a function try block for main(). In practice, because I use functions for all the “steps” of a program, I find that main() is so simple, the nested blocks aren’t such a big deal. For example, if I were writing your program, it might look like:

// Another Lippincott function, this one for program-specific error handling.
auto handle_error()
{
    try
    {
        throw;
    }
    catch (no_filename_argument_provided const&)
    {
        std::cerr << "Usage: <executable> -f <filename>\n";
    }
    // and any other error handling you want to do

    return EXIT_FAILURE;
    // note: no catch (...)
}

auto main(int argc, char* argv[]) -> int
{
    try
    {
        try
        {
            // Read the command line arguments and/or any configuration
            // files.
            //
            // handle_program_options() returns a struct or a map with
            // all the program options.
            auto const options = handle_program_options(argc, argv);
            auto const& filepath = options.filepath; // just for convenience

            // Create the sample data, and write it to the file.
            auto const original_data = create_data();
            write_data(filepath, original_data);

            // Read the data back from the file.
            auto const data = read_data(filepath);

            // Print the data.
            std::cout << "Printing contents of " << filepath << " file:\n";
            print_data(data);

            std::cout << "\nProgram terminated sucessfully. \n";
        }
        catch (...)
        {
            return handle_error();
        }
    }
    catch (...)
    {
        return indi::handle_uncaught_exception();
    }
}

TOCTOU

There is another general error-handling issue throughout your program, but this one is a higher-level, design-level issue.

Take a look at this bit of your program:

  if (!fs::exists(filepath)) {
    auto touch = ofstream{filepath};
    cout << "File " << filepath << " created." << endl;
  }

Now, suppose that the file path doesn’t actually exist. You call fs::exists(filepath), it returns false, then it gets negated, and the test passes, and you enter the block.

STOP! Right at that moment, the OS happens to suspend your process, and give another process some CPU time. This other process happens to create a new file… exactly with the same path you just tested. And then the OS scheduler suspends that process, and returns control to your process.

What happens now?

🤷🏼

This is UB. Literally anything could happen.

But let’s consider some realistic probabilities.

  1. The other process created the file path and filled it with critical info, and managed to do all that and close the file before it lost its time slice. Now your process, executes auto touch = ofstream{filepath};, thinking it’s creating the file… but instead opens the existing file, and truncates it… destroying that critical info. When the other process or something else goes looking for that critical info… again, who knows… could be a simple crash, could be something catastrophic.
    • Bonus confusion: The other process did the same thing you did: created the file, assumed it had rights to it, then closed it… then your process opens it, writes its integers, and closes it… then the other process re-opens it, writes its data, and closes it… then your process tries to read what it assumes are the integers it just wrote and… WTF?
  2. The other process never got around to closing that file before it lost its time slice—or maybe it did, but it had created it with restrictive permissions that your process doesn’t have. Either way, your process tries to open it… fails… but you never check… so it prints “File … created”. Later, you try to re-open it, and get the “cannot open” error. What the―? You can’t open the file you just created?

This is a general category of bugs known as “time of check/time of use” (TOCTOU) errors. It’s a race condition that pops up whenever you “check-then-do”, and outside forces can invalidate the check in the “then” part. Filesystem I/O is particularly vulnerable to this class of error, because you’re almost always sharing the filesystem with dozens or hundreds of other processes.

IOstreams is actually designed with TOCTOU in mind (all half-decent I/O libraries are, by necessity). Proper usage patterns for IOstreams are always “do-then-check”… not “check-then-do”. For example, this is bad:

if (not in.eof())
{
    in >> value;
    // use value
}
else
{
    // no more values
}

That is “check-then-do”. This is the proper “do-then-check” pattern:

if (in >> value)
{
    // use value
}
else if (in.eof())
{
    // no more values
}

So let’s rethink the way you’re creating your output file. You want to create the file if and only if it doesn’t exist, and write to it. If it does exist, you want to give the user the option to overwrite it, and if they say yes, open it for writing even if it does exist, stomping over its contents if any. I’ve formulated it this way so that all the I/O operations are done without checking. The code might hypothetically look like this (preserving all the messages of your existing code):

// First try to open exclusively.
auto out = xxx::ofstream{filepath, xxx::ios_base::binary | xxx::ios_base::exclusive};
if (not out.error_code)
{
    std::cout << "File " << filepath << " created.\n";
}
else
{
    // Failed to open for some reason.

    // If the reason is that the file exists...
    if (out.error_code == std::make_error_condition(std::errc::file_exists))
    {
        std::cout << "File " << filepath << " exists and cannot be created.\n";

        if (confirm("Do you wish to delete the file?"))
        {
            out = xxx::ofstream{filepath, xxx::ios_base::binary};
            if (not out.error_code())
            {
                std::cout << "Old file " << filepath << " removed.\n";
                std::cout << "New file " << filepath << " created.\n";
            }
        }
        else
        {
            std::cout << "Program exiting.\n";
            xxx::clean_fail();
        }
    }
}

// Check for any errors.
if (out.error_code)
    throw std::system_error{out.error_code};

// At this point, we can (try to) write to the file.

(As an aside, invalid_argument is not the correct error to throw when a file fails to open. This is an abuse of what invalid_argument means. There was (presumably) nothing wrong with the file path argument; it was a perfectly cromulent file path (if it wasn’t then the std::filesystem::path constructor should have failed)… it just couldn’t be opened for one reason or another. If there were something wrong with the argument, it would never be right… but if you can pass the same argument at different times, and it might open or might not, then the problem is not the argument, it’s something else.)

Unfortunately, the above is not possible with std::ofstream. There are two issues here:

  1. There is no exclusive flag… yet. I’m pretty sure it’s inevitable that it will be added; the committee just hasn’t got around to it. This is a major problem for this pattern.
  2. There no error code member to suss out the actual cause of the error. This is a less-major problem, because it primarily affects the quality of the error messages. As it stands in IOstreams, you can know whether something failed or not… but if it failed, you can’t find out why it failed.

For an example of where issue 2 pops up, consider this bit of your code:

  ifstream infile(filepath, ios::binary);
  if(infile){
    // ...
  }
  else{
    cout << "File " << filepath << " does not exist." << endl;
    // ...
  }

You assume the file doesn’t exist… but you have no way of knowing. All you know is you tried to open it, and that failed. Maybe it doesn’t exist. Maybe it does, but you don’t have permission to read it. Maybe it exists, and you have permission, but after you opened it, the stream failed to allocate something.

This is a general quality issue with standard IOstreams. You can’t give good error messages. You either have to accept that, and only give very literal, though very unhelpful messages (“failed to open file”… but why?!)… or you have to go beyond the standard streams.

And because of issue 1, you pretty much have to go beyond the standard streams… at least until the committee adds an exclusive flag.

So, yeah, in practice, for any non-trivial file I/O, you pretty much have to write your own file streams (technically, you have to write your own file buffer, but… yeah). Yes, that massively sucks. But that’s (one of the reasons) why people hate IOstreams. (And why there is work afoot to make a modern replacement.)

From this point on, I’m going to assume you are not going to try to create the output file exclusively. In other words, no “file exists, replace?” stuff… the user gave a filename, so it’s their responsibility if something gets overwritten. I’m also going to assume you don’t care about quality error messages. If these assumptions are wrong, you will have to be writing your own streams (and/or buffers), which is way beyond the scope here.

IOstreams error handling

Okay, now we get to the IOstreams specific stuff. When dealing with IOstreams error handling there are a couple things to keep in mind.

  • IOstreams is archaic. It not only predates the standard, it predates most of C++. It technically doesn’t predate C++ itself (C++ was renamed from “C with classes” about two years before IOstreams), but it predates the very first book (which was the unofficial standard for years), it was probably the very first C++ library written, and it predates just about everything that defines C++: templates, exceptions, and so on. Support for a lot of these things was retroactively grafted on to IOstreams… and it shows.

  • IOstreams is necessarily vague, in order to be portable (though, modern technologies like <system_error> do make it possible to be specific and portable). It doesn’t even assume a filesystem exists (and no, fstream et al do not require that a filesystem exists). This mostly comes up when trying to deal with errors. IOstreams will tell you that an error occurred… but they usually won’t tell you which error occurred.

  • IOstreams, like all half-decent I/O libraries, expects you to use the “do-then-check” pattern… not “check-then-do”. As the saying goes, it is better to ask forgiveness than to ask permission. Your code already does this (other than the stuff I mentioned previously), which is good. But do keep the pattern in mind. If you want something from a stream, just ask for it… don’t ask to ask.

  • IOstreams appears to make exceptions optional. Don’t be fooled. Even if you don’t explicitly enable exceptions in a stream, you can still get exceptions. That means you have to write all your IOstreams code to handle both exceptions and status flags. That gets clunky and frustrating, but it’s what you gotta do.

There are no “sticky” bits

IOstreams generally considers 2½ types of errors that map to bits in the std::ios_base::iostate enumeration:

  1. failbit: This is for “recoverable” errors… which really just means formatting errors. This is when the stream fails to convert the source data to the type you want (when reading) or fails to convert the data in your type to character data (when writing). The theory is that you could try to read some data as, for example, an int… and then if that fails, you can retry it as a string token. In reality… it’s not quite so simple, and it’s often impossible, or at least absurdly impractical, to recover from formatting errors. (To understand why, imagine the data is “123xyz”… you try to read as int, and successfully parse “123”, then fail… so you try to reparse as a string… but “123” has already been read, so you will only read “xyz”. To properly reparse “123xyz” as a string, you’d need to “putback” everything already read… which means you’d have to keep track of everything already read… which is pretty impractical, generally. I often do parsing using two (-ish) streams: one to read the entire “123xyz” token, and one to parse it. I never parse cin directly: I getline() into a string, and then try to parse that.)

  2. badbit: This is for “unrecoverable” errors, which generally means the stream itself is bad. Like the drive with the file you’re reading has been unplugged, or you were reading a network stream and the connection dropped, or whatever. This is considered “unrecoverable” because the only way to “fix” these errors is to give up, close the stream, and try to reopen it.

In practice, because retrying reading/writing operations is impractical, there’s usually no reason to distinguish between fail errors and bad errors. The exception is when you’re doing UI-like stuff with std::cin, because you want to distinguish between “user gave me garbage, but I’ll let them try again” (failbit) versus stuff like “the input pipe is broken” (badbit).

The “third” fail state is eofbit… which is hard to call a distinct thing because it always comes hand-in-hand with failbit. However, it does deserve special attention, because there are some cases where you want to separate eof from fail:

  1. When you are reading an unknown number of “things”, you will eventually get a failure. If eofbit is set, that means you successfully reached the end of the “things”. Otherwise, you did not reach the end, but there is corrupt data.
  2. After you have read all the data you expect, and you want to make sure there isn’t any trailing garbage, you can try to read one more character. It should fail, and set eofbit. If it does not fail, or it fails but does not set eofbit (which should never happen), then you know there is trailing garbage.

The latter case is relevant to your program: after you successfully read the data, you should check to make sure you read all the data:

if (auto in = std::ifstream{filepath, std::ios_base::binary}; in)
{
    auto buffer = std::array<int, 10>{};

    // others have explained why this is not correct, but whatever
    if (in.read(reinterpret_cast<char*>(buffer.data()), buffer.size() * CHAR_BIT))
    {
        std::cout << "Reading from file " << filepath << " succeeded.\n";

        // we read the data... but did we read *all* the data?
        if (in.peek(); not in.eof())
            throw std::runtime_error{"unexpected extra data in file"};
    }
    else
    {
        throw std::runtime_error{"Reading from file " + filepath.string() + " failed."};
    }
}
else
{
    throw std::runtime_error{"File " + filepath.string() + " does not exist."};
}

So, the title of this section is “there are no ‘sticky’ bits”. Let me clarify. When we talk about the “sticky” bits, we mean the FORMATTING bits… not the status bits. The reason is because formatting flags are usually ephemeral in, like, every other I/O library every written. IOstreams is very weird in that regard. For example, consider C’s stdio library:

std::printf("%x", 42);  // print an int formatted in hexadecimal
std::printf("%d", 42);  // print an int (note, hex format is not "sticky")

But with IOstreams:

std::cout << std::hex << 42;    // print an int formatted in hexadecimal
std::cout << 42;                // print an int (hex format is "sticky"!)

If you want to be pedantic, the status bits are, technically, “sticky”. But we (usually) never talk about them that way… because what else would they be? Consider what it would be like if the status bits weren’t “sticky”:

if (std::cin >> x >> y >> z)
    // successfully read x, y, and, z!

// ... or did we?
//
// what if there was an error reading x, but then when reading y, the
// status bits were reset, and y was read successfully? you'd never know
// there was an error. x would just have some default value; you'd never
// know if that was what was actually read for x or not.

You should (almost) never use .good()

There are at least two ways to check for IOstream errors, the bool conversion, and .good(). The only difference is that .good() also checks eofbit. In other words:

// these two expressions are *exactly* equivalent
s.good()
(s or s.eof())

So since .good() checks everything the bool conversion does… and more… doesn’t that mean it’s better? Doesn’t that mean it should be the default choice?

No, but the reason why isn’t technical, it’s social.

When you write code, you are obviously trying to communicate with the compiler, and get it to generate operations that do what you want. But you are also communicating with your peers. A lot of the time, there are multiple ways you can write your program that all mean the same thing to the compiler… but send very different messages about your intentions to other programmers.

Now, in IOstreams, EOF always happens with failure. This is a consequence of the TOCTOU-proof design of IOstreams; you don’t get EOF until you actually try to read past the end of the file, at which point the read was obviously a failure. So you (almost) never need to check for both EOF and failure, making .good() redundant. You just need to check for what you actually care about: whether EOF or failure. And the thing you choose to check for reveals your intentions about the code.

Consider the following code (which is a very stripped down version of your input section):

if (auto s = std::ifstream{path}; s)
{
    if (s.read(/* ... */)) // or if (s.read(/* ... */; s)   // <-- 1
    {
        if (s.peek(); s.eof())  // <-- 2
            // everything was okay!
    }
}

At point 1, I use the bool conversion, which sends the message to readers that what I really care about is whether the read operation succeeds or not. I am not interested in whether the reason for the failure was that the file was empty or not. I want that data, and all I care about is whether I get it or not.

At point 2, I ask for .eof(). This sends a very different message. It says that I am only interested in whether I was at the end of the file. If I’m not, I don’t care whether the peek succeeded or failed… either way, I wasn’t at the end of the file, which is what matters to me.

I could rewrite the code above to use .good() for every test. It wouldn’t change the logic of the program in any way (well, except the last test would need to distinguish between EOF and failure). But if I did that, it would obscure my intentions in the code.

You could basically replace every bool test of a stream with .good() in your code, or vice versa. It won’t change the behaviour of the code. But if you use .good() everywhere, you are implying to me that you care about EOF everywhere… even though you don’t. You would be miscommunicating your intentions… which is bad, because the whole point of using a high-level language is to communicate your intentions clearly.

To put it another way, if I saw code where someone had used .good() everywhere, I won’t be thinking “this programmer is being cautious and defensive in their code”. I would be thinking “this programmer doesn’t know what they’re doing (or, at least, they’re not telling me what they actually mean)”.

So does that mean .good() is completely pointless? Is there never a situation where you should use .good()?

I can only think of one situation where .good() makes sense.

Suppose you were doing an I/O operation that involved some very expensive stuff. For example, suppose you were doing an input operation that involved allocating a large buffer. If the stream is not in a good state for reading, you want to avoid that expensive allocation… you want to “fail fast”. In that situation, you could write:

auto expensive_input_operation(std::istream& in)
{
    if (in.good())
    {
        // do expensive operation
    }
    else
    {
        // Why set the failbit? Because it is the right thing to do whenever
        // your input/output operation fails.
        //
        // The failbit *might* already be set... but it might not. For
        // example, the stream might actually be bad (badbit). Or someone
        // mucked with the status bits.
        //
        // Either way, all you care about in this function is that your job
        // was to do some input, but you couldn't... so, set the failbit.
        in.setstate(std::ios_base::failbit);
    }
}

You can’t—or rather, shouldn’t—just use the bool conversion here, because it is conceivable that someone cleared failbit and badbit, but failed to clear eofbit. But even if there were no difference, the use of .good() here sends the message that you really just care if the stream is in a good state. If it’s not, you don’t care why not; you don’t care what kind of bad state it’s it. It’s either good, or you don’t care.

This is the only situation I can think of where .good() makes sense: when you’ve just been handed a stream from god-knows-where, and you’re about to do some expensive work involving that stream, and you want to skip that expensive work if it’s doomed to be pointless anyway.

(In practice, when I’ve done really complicated and expensive I/O functions, I’ve usually ended up using the stream buffer, rather than the stream. That means using the stream’s sentry, which makes .good() pointless in that case, too. But that’s getting really deep into the weeds.)

Putting it all together

So, here’s a link to an implementation of your program I put together. It’s not exactly like your program, as mentioned in the comments—it doesn’t check for whether the output file exists first. But it’s as close as I could get with pure, standard IOstreams.

I’m not claiming this is the “right” way to write this code. I just want to illustrate how it’s possible to almost completely remove error-handling from the happy path. (Except for the handle_program_options() function, which is a mess, because it’s just hacked together; it’s not realistic argument-handling code.) Like, when you’re reading the code to find out what it does, you start in main(), say: “Okay, step 1 (after command-line argument handling) is writing data (as implied by the function being named write_data()). How do we do that? Well, as it says in write_data(), if the output file opens, and if the serializing succeeds, then we print a success message.” While the code handles all errors, and prints human-readable messages for them all, all of that happens out-of-the-way from the main logic.

And if writing the data were more complicated, the actual code wouldn’t get more bogged down with error-handling stuff. For example:

auto write_data(std::filesystem::path const& path, ...)
{
    auto out = std::ofstream{path, std::ios_base::binary};
    if (not out)
        throw failed_to_open_output_file{path};

    if (not (/* output operation 1 */))
        throw failed_to_do_output_operation_1{};

    if (not (/* output operation 2 */))
        throw failed_to_do_output_operation_2{};

    // ... and so on ...

    if (not out.flush())
        throw failed_to_write_to_output_file{};

    // output file is automatically closed

    // Note that there is no portable way 100% absolutely guarantee that
    // the data was actually written to disk in a way that there is an
    // uncorrupted file on that disk that can be reopened later, and
    // will contain that exact data.
    //
    // But if the flush operation succeeds... that's *probably* good
    // enough. You don't really need to check that *closing* the file
    // succeeds. In most real-world programs, you usually don't close a
    // file then reopen it in the same program, so in practice, even if
    // a close fails, the file handle/id will not be "stuck" holding the
    // file open.
}

Input code looks pretty much the same.

Note that the pattern means always checking for failure… not success. Checking for success looks reasonable if you’re not using exceptions, because it puts the happy code first, and the error code is pushed out of the way in a later else block. But if you always check for success, you’d get:

auto write_data(std::filesystem::path const& path, ...)
{
    auto out = std::ofstream{path, std::ios_base::binary};
    if (out)
    {
        if (/* output operation 1 */)
        {
            if (/* output operation 2 */)
            {
                if (/* ... and so on ... */)
                {
                    if (out.flush())
                    {
                        // ...
                    }
                    else
                    {
                        throw failed_to_write_to_output_file{};
                    }
                }
                else
                {
                    // ... more errors ...
                }
            }
            else
            {
                throw failed_to_do_output_operation_2{};
            }
        }
        else
        {
            throw failed_to_do_output_operation_1{};
        }
    }
    else
    {
        throw failed_to_open_output_file{path};
    }

    // output file is automatically closed
}

Which is ugly and unscalable.

Even if you’re not using exceptions, you should still consider the test-for-failure-and-bail pattern over the test-for-success-with-an-else pattern. It should’t be a problem in C++, because you can (and should) use RAII to clean up intermediate stuff, so the fail blocks would just be return fail_status; rather than throw whatever;. (Of course, you only have to write x; if (not x_succeeded) throw y; if x doesn’t throw on failure itself… which it should. IOstreams is just too ancient to do that.)

If you don’t care about getting specific error information, this could be even simpler:

auto write_data(std::filesystem::path const& path, ...)
{
    auto out = std::ofstream{path, std::ios_base::binary};
    out.exceptions(std::ios_base::failbit | std::ios_base::badbit);

    /* output operation 1 */

    /* output operation 2 */

    // ... and so on ...

    out.flush();

    // output file is automatically closed
}

Personally, I like to use the pattern where the file stream is restricted to a block, because it’s just nicer to keep the scope of resources as small as possible:

if (auto stream = std::ifstream{path}; stream)
{
    // stream object only exists within this block
}
// optional if you want to do something about failure:
//  else
//  {
//      // handle failure to open file
//  }

// file is already closed and stream resources cleaned up by this point

A small aside that is actually a HUGE rabbit hole

There is a big chunk in the middle of your code that looks like this:

    string yesOrNo = {""s};
    bool isFalse = 1;
    do {
      cout << "Do you wish to delete the file? Type yes/no and return." << endl;
      cin >> yesOrNo;
      for_each(yesOrNo.begin(), yesOrNo.end(), [](char& c){c = tolower(c);});
      if (yesOrNo == "yes"s) { 
        fs::remove(filepath);
        cout << "Old file " << filepath << " removed." << endl;
        auto touch = ofstream{filepath};
        cout << "New file " << filepath << " created." << endl;
        isFalse = 0;
      }
      else if (yesOrNo == "no"s) {
        cout << "Program exiting." << endl;
        exit(-1);
      }  
    }while (isFalse);

Now if this were being refactored, all that yes/no code would be moved into its own function, so this might become something like this:

    if (confirm("Do you wish to delete the file?"))
    {
        fs::remove(filepath);
        std::cout << "Old file " << filepath << " removed.\n";
        [[maybe_unused]] auto touch = std::ofstream{filepath};
        std::cout << "New file " << filepath << " created.\n";
    }
    else
    {
        std::cout << "Program exiting.\n";
        // exit program somehow
    }

That confirm() function seems innocuous. As a first pass, you might write it like this:

auto confirm(std::ostream& out, std::istream& in, std::string_view const question) -> bool
{
    auto const is_yes = [](std::string_view s) { /* check for "y" or "yes", case insensitively */ };
    auto const is_no  = [](std::string_view s) { /* check for "n" or "no", case insensitively */ };

    auto input = std::string{}; // moved outside the loop so we can reuse the memory
    while (true)
    {
        out << question << " [yes/no]: ";

        in >> input;

        if (is_yes(input))
            return true;
        else if (is_no(input))
            return false;
        else
            out << "Sorry, your response was not understood.\n";
    }
}

auto confirm(std::string_view question) -> bool
{
    return confirm(std::cout, std::cin, question);
}

This is almost exactly your existing code… the only real additions are that it emits a “sorry, don’t understand” message before re-asking for confirmation, and, of course, std::cin and std::cout aren’t (necessarily) hard-coded, so you can test the function.

However, this function is buggy, and has no error checking whatsoever. Thing is, fixing it is such a massive undertaking, that requires going so deep into IOstreams quirks, that it would require a whole new answer almost the same size as this current answer… which is already huge. I’m actually seriously considering writing an article on how one might implement this kind of function—that’s how complicated fixing this would be.

It’s always the “simple” things that are the most devilishly complicated.

Summary

So it’s kinda hard to summarize all the details of working with IOstreams error handling, but I’ll try.

  1. IOstreams makes exceptions “optional”, but you have to write all your code assuming they’ll fly anyway. That means that when you’ve been handed a stream (as a function parameter, for example), you either have to:

    a. manually enable exceptions in the stream so you know what you’re dealing with

    b. manually disable exceptions in the stream so you know what you’re dealing with… but safely disabling exceptions is tricky (and you’ll have be ready for exceptions regardless); or

    c. write your code in such a way that it handles both stream exceptions and status flags.

    In general, the practical options are either b or c.

  2. IOstreams works under the “do-then-check” paradigm; don’t ask permission, just do what you want and then check the status flags to see if it worked. It usually won’t matter if the stream was already bad (unless you’re trying to narrow down which operation failed), so there’s generally no need to check for that in advance.

  3. IOstreams recognizes 2½ types of failure:

    a. “Recoverable” failures: failbit. These are generally failures converting to/from character data, so parse/format failures. They’re called “recoverable” failures by the standard, but in practice, they’re usually impractical to recover from.

    b. “Non-recoverable” failures: badbit. This means the stream itself is broken. (For example, the drive you were reading/writing was unplugged.)

    c. End of file (EOF): eofbit. This is not really an error, per se, but it’s packed in with the failure status. It always comes in tandem with failbit (though it is possible to use .setstate() to set eofbit without setting failbit, if you are being nefarious), so it doesn’t really deserve to be considered on its own, except that sometimes you want to distinguish between failure due to corrupt data and failure due to no more data.

  4. Avoid .good(), because it is too broad. Use either the stream’s bool conversion to check for failure (and .bad() or .fail() if you are interested in the specific type of failure, though that is very rare), and .eof() to check for EOF.

  5. Realize that IOstreams, out of the box, is very limited. One way it is limited is in determining the exact cause of a problem—it will tell you when failure occurs, but it usually won’t tell you why.

    However, while it is limited out of the box, IOstreams is astonishingly flexible and extensible. It’s not easy to extend… but if you’re willing to do the work, it will pay off.

  6. A lot of the time, it’s easier to separate input and parsing, particularly when using cin for input, because if parsing fails, cin goes into fail state… which is usually problematic. Instead, especially when dealing with line-oriented input, it usually makes more sense to just read in the whole line from cin into a string, and then parse that (either with istringstream or from_chars() or whatever). If the parsing goes bad, cin is still okay, so re-asking for the information is trivial.

  7. If you want to confirm that writing succeeded, you should probably flush and then check. Flushing doesn’t guarantee that the output was successfully written to a file or whatever—that level of guarantee requires very low-level OS support. But it’s close enough.

    If you want to confirm that reading succeeded… well, it either did, or it didn’t. There’s no trick there.

That’s about all I can think of. Happy streaming!

\$\endgroup\$
1
  • \$\begingroup\$ I appreciate the time you invested and I couldn't accept your answer! Thanks for making my dull train ride interesting. I might have understood less than half of what you wrote, but there is lots of useful information for me to study in your answer. \$\endgroup\$
    – AngelosFr
    Nov 20 at 21:15
11
\$\begingroup\$

using namespace std;

Don't do using namespace std;. It can lead to name collisions and other issues.

Typing out the full names (e.g. std::cout) avoids such issues, and is also clearer for readers.

using namespace std::literals; is reasonable inside a .cpp file, or a function.


string_view literals

std::string_view filename{""s};

s is the std::string string literal, so this is unsafe (it creates a string view into a temporary string). We should use the std::string_view literal, sv, or simply leave the variable as a default initialized (empty) string-view.

argv[i] == "-f"s

Again, we should use "sv" - we don't need to actually construct a std::string, a std::string_view of a string literal (effectively a raw character array) is fine.


use functions

The main function is very long, and could be easily split into smaller parts that each do one specific task. This is important because it limits the scope of local variables and makes dependencies much more obvious.

Basically where each of your comments are:

/* Command line arguments */
/* Create file if it does not exist */
/* Open file and write to it */
...

These could be calls to functions with names like read_command_line_args.


simplifying file usage / user interation

if (!fs::exists(filepath))
{
    auto touch = std::ofstream{filepath};
    std::cout << "File " << filepath << " created." << std::endl;
}
else
{
    std::cout << "File " << filepath << " exists and cannot be created." << std::endl;
    std::string yesOrNo = {""s};
    bool isFalse = 1;
    do
    {
        std::cout << "Do you wish to delete the file? Type yes/no and return." << std::endl;
        std::cin >> yesOrNo;
        for_each(yesOrNo.begin(), yesOrNo.end(), [](char &c) { c = (char)tolower(c); });
        if (yesOrNo == "yes"s)
        {
            fs::remove(filepath);
            std::cout << "Old file " << filepath << " removed." << std::endl;
            auto touch = std::ofstream{filepath};
            std::cout << "New file " << filepath << " created." << std::endl;
            isFalse = 0;
        }
        else if (yesOrNo == "no"s)
        {
            std::cout << "Program exiting." << std::endl;
            std::exit(-1);
        }
    } while (isFalse);
}

I don't think any of this is necessary. It makes the program more complicated for both the programmer and the user. We already made the user specify a command line argument, so the extra interaction isn't needed.

We should either:

  • require that the user specify a file that doesn't exist.
  • assume that they know what they're doing and overwrite the specified file (i.e. use std::ios::trunc when opening the file for writing).

Note that we don't need to "touch" a file to create it. We can just open it and start writing.


writing binary data

outfile.write(reinterpret_cast<char *>(&bufToWrite), sizeof(bufToWrite));

This is simple enough that it works when writing a file on a particular machine, and reading that file on the same machine. However, the binary file written isn't portable between computers because of several issues:

  • Size of types - int is not a fixed size type - for example it could be 32 or 16 bits on different machines.

  • Numeric representation - different machines may represent signed numbers differently in memory.

  • Alignment - different machines lay out combinations of simple types differently in memory. This shouldn't affect an array of integers, but more complex types (e.g. an array of structs containing member variables of various different types) may have their parts located in differing locations in memory, and have padding in between the members.

  • Endianness - different machines may store bit representations of numbers in different directions.

Each of these issues needs to be solved before we can portably read a binary file written on a different machine. This can be done by:

  • Write only fixed size types (e.g. std::uint32_t etc. from the <cstdint> header).

  • Cast fixed-size signed numbers to the same-sized unsigned type before writing.

  • Don't write aggregate types in bulk - write elements individually (unless you're certain you can get away with doing otherwise).

  • Convert types to a known endianness when writing. Each machine can then convert back to its own system endianness when reading the file.


closing files after writing

if (outfile.good())
{
    std::cout << "Writing to file " << filepath << " succeeded." << std::endl;
    outfile.close();
}

Note that closing a file can also fail, so we should check for failure afterwards (files are buffered, so writing to disk might not happen until we close the file).


\$\endgroup\$
10
  • 2
    \$\begingroup\$ Should I use good() after closing a file then? How can I make sure that the error, whatever might that be, will be identified at the moment of occurrence and not at a later time due to the intermediate buffer? \$\endgroup\$
    – AngelosFr
    Oct 20 at 14:26
  • 3
    \$\begingroup\$ @AngelosFr Yep. close() sets the failbit if something goes wrong, so you can use good() (see: en.cppreference.com/w/cpp/io/basic_ofstream/close ). write() errors set the badbit (and if the badbit is already set, the failbit). You can check after every write or once at the end, it's up to you. Note that you can also set the exceptions flags to get an exception if anything goes wrong: en.cppreference.com/w/cpp/io/basic_ios/exceptions \$\endgroup\$
    – user673679
    Oct 20 at 15:20
  • 4
    \$\begingroup\$ This is a good answer, but there is a lot of important stuff missing from it. Some of it is low level problems (use of std::exit(), throwing exceptions with no catch, etc.), and some of it is high-level design issues (the filesystem race conditions, all the bugs (command line parsing, yes/no logic) etc.)… but most notably, the questions asked were never really addressed (like when to use good() (never), when to use the sticky bits (a non-sequitur), etc.). I guess the questioner was satisfied with the answer, though, so… 🤷🏼. \$\endgroup\$
    – indi
    Oct 21 at 2:24
  • 2
    \$\begingroup\$ @indi I'm always happy to read your answers, so I'd love to see your take on things. I'm curious about what you mean by never using good(). \$\endgroup\$
    – user673679
    Oct 21 at 7:21
  • 3
    \$\begingroup\$ Oh, I didn’t mean that good() is wrong. It’s just that it’s pretty much never necessary. It’s almost always simpler to use the bool conversion to check the stream state after any operations. Like, the output here, with checking, is basically a one-liner: if (output.write(...).flush()) /* write *probably* succeeded (not accounting for OS/hardware/other buffering) */. And the input is basically: if (infile.read(...)) /* read succeeded (though there might be other stuff in file) */. Wrap those in scopes (or better, functions) to auto-close, and that’s all you need. \$\endgroup\$
    – indi
    Oct 21 at 10:11
8
\$\begingroup\$

Also,

⧺SL.io.50 Don't use endl.

Don't explicitly compare against nullptr. Use the truth value of the pointer (or smart pointer!): if (argv[++i]). In this case, modifying the index as part of the long expression that refers to it several times is confusing. I get the idea that this is boilerplate code anyway, so use a proper library instead of pasting it.


size_t bufSize = sizeof(bufToWrite)/sizeof(DType);
DType toRestore[bufSize] = {};

First of all, that second line is not actually legal in C++. gcc accepts C-style VLA's as an extension with no warning, but incorporating this into the C++ standard was rejected for good reason.

Don't use sizeof on an array. It is fragile and can easily accidently give you the size of a pointer instead. Here you just want the number of elements in the array: that's std::size(bufToWrite).

But really, you just want to duplicate the type of bufToWrite in both its array size and element type. So just write "I want another variable the same as that one" directly:

decltype(bufToWrite) toRestore;

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Which library can I use for example to handle the first part of the code with the command line arguments? Is there anything in the standard library? \$\endgroup\$
    – AngelosFr
    Oct 20 at 14:23
  • 3
    \$\begingroup\$ @AngelosFr I use Clara, and it's also part of Catch2 which I use for writing unit tests. \$\endgroup\$
    – JDługosz
    Oct 20 at 14:28
  • 1
    \$\begingroup\$ General question: Would you (if you were to write this) use iostreams at all, or rather the C standard library, or potentially immediate POSIX low-level I/O? \$\endgroup\$ Oct 22 at 7:02
  • 1
    \$\begingroup\$ @Peter-ReinstateMonica Good question! In the distant past, writing for 16-bit DOS, I used low-level OS wrappers (meant to look like the POSIX functions). On later machines, the benefits of the library's I/O buffering showed up. I think I'd look into the standard library's implementation and performance: does iostream add another wrapper around stdio, or do they both use a common back-end? I'd prefer the iostream because the fstream has a destructor and it knows about filepath rather than C-style strings. If performance became a bottleneck, I'd look into it. \$\endgroup\$
    – JDługosz
    Oct 22 at 16:17
  • \$\begingroup\$ @Peter-ReinstateMonica What I really like is to know that the entire record to be parsed is in a contiguous memory buffer, so it can just advance the pointer rather than having to issue a read call at each little part. Loading the whole file into memory and then parsing that; or designing the data so the records have length prefix, I'd make the unpacking code take the memory block and not care about files at all. Then it's easy to change the layer that opens and reads (or receives from the network, or locates a resource, or...) \$\endgroup\$
    – JDługosz
    Oct 22 at 16:21
7
\$\begingroup\$
  1. Style:

    using namespace std;
    

    This is generally seen as poor practice because it imports all symbols from the std namespace in the main namespace, somewhat defeating the namespace isolation and highly increasing the risk of name collision. Explicitely importing only the used symbols is generally seen as better practice, even if it requires more typing:

    using namespace std::string_literals;
    namespace fs = std::filesystem;
    using std::cout
    ...
    

    if ((argv[i] == "-f"s)
    

    Nothing is wrong here: "-f"s is indeed a std::string so the compiler should select the string equality operator. But it could unsettle older reviewers which were used to the C language. Said differently, if everybody in your team is using that idiom, just keep on, if you are working on an opensource project, or expect older reviewers, you should add a comment.


    You sometimes test if (stream) and sometimes if (steam.good()). It is not an error but is useless because when used in boolean context, a stream is converted to true iif it is in good state. The idiomatic way is to consistently test if (stream). If you want to reset a bad read condition you have to clear it with stream.clear()


    You consistently use endl to write a newline on stdout. In fact using endl both adds the newline and flushes the output stream. It is probably not a problem here but it can prevent best performances when writing to a real file.

  2. IO testing

    The loop asking whether an existing file should be removed fails to test all possible bad conditions:

     do {
       cout << "Do you wish to delete the file? Type yes/no and return." << endl;
       cin >> yesOrNo;   // what on read error?
       ...
     }while (isFalse);
    

    If cin finds an error (whatever the reason) you will enter an endless loop because yesOrNo will not be updated and will never get a yes or no value

  3. File system handling

    If the file already exists, you remove it and try do create it again. Write access to a file and permission to remove and create it are different permissions (for example in a plain Unix-like filesystem, write access to a file is the w permission on the file, removing and creating it requires w permission on the directory). If you know that it cannot be a problem in your use case, just move on, but you must be aware of it.

    When writing to the file, you use ios::in | ios::out, while you never read from that stream.

  4. Variable Length Array in C++

    You use

     size_t bufSize = sizeof(bufToWrite)/sizeof(DType);
     DType toRestore[bufSize] = {};
    

    unless you declare bufsize to be constexpr it is a normal variable and not a constant, so toRestore is a VLA. Both gcc and clang do allow VLA as documented extensions, but the standard does not, and there are good reasons for that. Just declare busSize to be constexpr

\$\endgroup\$
9
  • 3
    \$\begingroup\$ re "nothing wrong here" for (argv[i] == "-f"s): it's legal and does what is expected, but it's inefficient to construct a string just to do the comparison. It should be written "-f"sv, as in other places where the temporary string is actually wrong. I actually advocate using sv literals for comparing against zstrings, as a replacement for strcmp. There's so much more for "older reviews" to be unsettled about and I'm not pandering to them! \$\endgroup\$
    – JDługosz
    Oct 20 at 14:33
  • 2
    \$\begingroup\$ @JDługosz: I was trying to bring my help and recieved yours :-) Thank you, even old dinosaurs like to learn... \$\endgroup\$ Oct 20 at 14:36
  • 2
    \$\begingroup\$ So is the if(file) the one line to deal with everything? Either to check file existence or a bad state after an IO operation? \$\endgroup\$
    – AngelosFr
    Oct 20 at 14:37
  • 2
    \$\begingroup\$ @SergeBallesta thanks. I keep meaning to write a Code Project article on this, but never finish it. \$\endgroup\$
    – JDługosz
    Oct 20 at 14:42
  • 3
    \$\begingroup\$ @AngelosFr read en.cppreference.com/w/cpp/io/basic_ios/operator_bool It is defined as "Returns true if the stream has no errors and is ready for I/O operations." sounds like exactly what you need to check. \$\endgroup\$
    – JDługosz
    Oct 20 at 14:43
2
\$\begingroup\$

The other answers give good information as to what you're doing is wrong and specifically how to correct your issues, but they miss the larger picture:

Don't write binary data to files in C++

As @user673679 pointed out there are many possible portability issues with writing raw binary in C++ like endianness, memory layout, etc. Instead of trying to mitigate those issues like they suggested though, the correct thing to do in 99% of cases for robust, industry standard, code is to use a standardized network transcriber like Protobuf.

Google Protocol buffers, also known as Protobuf, is an industry standard language agnostic way of of writing rich data to binary. It will likely be a hair slower than direct bit writing so in maybe 1% of cases the way they suggested is necessary, but this will cover every possible edge case for transcription and most importantly be quite readable and portable.

The main downside is having to maintain an external dependency which is much worse for C++ than basically any other language, but I think it's very much worth it.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Protobuf turns a data structure into binary data. Then you have to write that binary data to a file... \$\endgroup\$
    – Ben Voigt
    Oct 21 at 16:44
  • 1
    \$\begingroup\$ @BenVoigt Well, with a bit of benevolence one would read the bold sentence as "Dont't write native typed data directly as binary", or something like that. Obviously, all data is binary at the end of the day ;-). \$\endgroup\$ Oct 22 at 6:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.