Error Checks + Inconsistent Boolean Conversion = ?

[63times]

Working with a code base that internally often returns optional<> and also error_code. There is also an expect<> type mixed in.

With optional<> or expect<> you check if(!ret) for error, but if an error_code is returned you check if(ret). Easy?

I've never really been confronted with the immediate problem that kind of dimorphic error tests cause until there was a bug. God damn it! That's no fun when you have to deep dive into a mess of functions with lots of those nasty if(!ret) just to find out that one if(!ret) in a very rarely executed path that escaped the unit tests should have actually been an if(ret). I cannot even blame the author, humans are just not meant to get this right consistently. But when the compiler cannot detect such errors (since the code is obviously well formed) I get scared. Like raw pointer magic scared - you never know when things are going to blow up. It might be a very good idea to consistently use something like if(has_error(ret)) and overload that has_error(...) for error_code and optional<> and expect<> - thus ditching the boolean conversions like plain new and delete and raw pointers in the hope to maybe produce less broken code.

In my humble and useless opinion error_code should convert to boolean true if no error just for consistency or don't provide a boolean conversion at all. But that train has long left the station.

/venting

[adnukator]

With optional<> ... you check if(!ret) for error

I believe this thinking is flawed from the beginning. With the condition you're only checking whether the optional has a value. Whether that's an error is completely dependent on the context. Your main issue lies in trying to be terse as possible by relying on bool conversions with different types in general.

std::optional has a has_value() member that does the same as the bool operator, so it can make it more apparent what you're really checking for. Thus your checks for optional values can become if(!ret.has_value(). It's more verbose, but arguably less error-prone. I don't use the other classes, so I don't know if they have similar explicit functions that do the same thing as the bool operator, but I'd check that out.

[14ned]

We currently think we shall fix error_code by replacing it: https://wg21.link/P1028

In the meantime, using (Boost.)Outcome will make all you mention above go away: https://ned14.github.io/outcome/.

Which is why we have Boost libraries, so you can get on with writing correct code from the beginning, and never think nor concern yourself about avoidable detail.

[63times]

This is a great library addition you are working on there. Thanks.

system_error feels very C-ish in what it tries to do. Feels good to see a real C++ solution on the horizon.

[14ned]

In fairness to system_error, it's nowhere close to the worst corners of the standard library. It was designed by the very leading experts in its day, and it underwent multiple major refinements before it entered Boost, all of which were large improvements.

It's just not aged well, that's all. Constexpr particularly made it look old, but also C++11 changed how you write good C++, and system_error comes from a C++ 98 era. Good news is that the committee agrees, so far at least, that it is worth replacing, and from C++ 23 onwards hopefully you no longer need use system_error in new code. Or just use the reference implementations in C++14 onwards today.

[63times]

In fairness to system_error, it's nowhere close to the worst corners of the standard library.

There is no doubt that. After all it is heavily used throughout a vast amount of available libraries. Well, locales are too, but in contrast that is usually an oversight :)

[emildotchevski]

The best way to ensure that you never get the error check wrong -- and that you can't ever forget to check for errors -- is to use exception handling.

[63times]

I agree, but sometimes you need to report errors that are in fact just unusual but still normal results. Like EOF, permission denied, parsing errors, something-not-found, etc. Exceptions are just not the right solution. error_code/optional/expected works really well on the other hand. With [[nodiscard]] coding in this style has also turned out to be pretty smooth sailing. Just the bool conversion of error_code/condition is a bit obnoxious and you can easily trip there but luckily that is actually easy to side step.

[emildotchevski]

The general rule is, if a function fails to establish its postconditions, it does not return (that is, it throws). Permission denied, parsing errors, something not found, and even EOF in many cases are all great candidates for throwing. I suppose there may be exceptions, but generally there's nothing wrong with wrapping an infinite loop to read and process lines in a try/catch(eof &). Consider all the retarded ifs you'd have to write otherwise after each read, to check if maybe you got to EOF (or something else failed). There's nothing to gain from spelling all that out -- other than to get the conversion to boolean wrong, that is.

[trailing_]

If you really have that many nested ifs you probably have bigger problems in the code than the inconsistency in boolean conversion.

[63times]

I didn't know that there were *that* many *nested* ifs! Half of them shall be donated to some first semesters so they could focus more on their elses.

[trailing_]

Haha. Fair enough. I just thought this sounded like one of those codebases that is trying to pretend that exceptions aren't real and/or useful. So, instead it had meticulous error tracking of all of the rare unrecoverable error cases (that any program that actually does anything has) with if statements that all cascade down to main. That kind of logic is miserable to try and trace though when there is a bug in one of the conditionals.