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 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…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.
- 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?
- 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:
- 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.
- 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:
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.)
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:
- 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.
- 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.
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.
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.
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.
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.
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.
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.
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!
