RFC on an approach to address the "return boolean or throw exception" quandary

[gardhr]

After reading manasij7479's thread on throwing exceptions in constructors, I got to thinking about the related issue in general of deciding whether to throw an exception or return boolean values from certain functions. It then dawned on me that there may be a way to "have one's cake and eat it too": design a class that throws an exception if unchecked, or simply reports it's status otherwise. So here's what I've come up with so far (just keep in mind that the code is only a few hours old, so it probably contains bugs):

/*
	A possible candidate for propagating state-integrity from functions. 
	Note: the const-ness of rvalues of this class is NOT preserved 
	(necessary to make everything work properly, unfortunately)
*/
class status
{
	public:
		
	enum 
	{ 
	/*
		Give a few of options for the most common naming choices
	*/
		success, 
		good = success, 
		failure, 
		fail = failure, 
		bad = fail, 
	/*
		User-defined codes should start at status::define
	*/
		define 
	};
	
	status(int code = success, bool throws = true)
	{
		(*this)(code, throws);
	}
	
	status(bool state, bool throws = true)
	{
		(*this)(state, throws);
	}	
	
	status(const status& other)
	{
		*this = other;
	}	
	
	status& operator () (int code, bool throws = true)
	{
		this->code = code;
		this->throws = throws;
		return *this;
	}

	status& operator () (bool state, bool throws = true)
	{
		return (*this)(state ? success : failure, throws);
	}
	
	operator bool ()
	{
		throws = false;
		return code == success;
	}

	operator int ()
	{
		throws = false;
		return code;
	}
	
	bool operator == (const status& other)
	{
	/*
		We'll use this type of comparison in the event that 
		derived classes decide to use bitmasks for error codes
	*/
		return code & other.code;
	}
	
	bool operator != (const status& other)
	{
		return !(*this == other);
	}	
	
	status& operator = (const status& other)
	{
		this->code = other.code;
		this->throws = true;
		const_cast<status&>(other).throws = false;
		return *this;
	}	
	
	virtual ~status()
	{
		if(code != success && throws)
			throw status(this->code, false);
	}		
	
	/*
		Probably wouldn't hurt to make this public?
	*/	
	int code;
	
	protected:
		
	bool throws;
};

And here's an example of it being used:

#include <iostream>

class example
{
	public:
		
	status broken()
	{
		return false;
	/*
		Alternately:
	*/
		return status::fail;
	/*
		...But NOT this, as we would be passing an int (not a bool) to the constructor:
	*/
		return 0;
	}

	static void report(int pos, bool state)
	{
		std::cout << "(" << pos << "): " << (state ? "success" : "fail") << std::endl;
	}	
};

int main(void)
{
	example test;
	try
	{
	/*
		Status NOT checked, so exception is thrown (but eventually caught)
	*/
		test.broken();
	}
	catch(status&)
	{
		example::report(1, false);
	}
/*
	If 'state' is never checked by the time it goes out of scope, an exception will be thrown
*/
	status state = test.broken();
/*
	Okay, 'state' is checked, so NO exception is thrown
*/
	example::report(2, state);
/*
	Status checked, so NO exception is thrown
*/
	example::report(3, test.broken());
/*
	Status NOT checked, so exception is thrown (but not caught, so program terminates)
*/
	test.broken();
/*
	We never get this far, of course
*/
	example::report(4, false);
}

Any comments, suggestions, or bug-fixes would be appreciated!

[gardhr]

I'm starting to think that the mixed use of booleans and integers may just make things too error-prone. I'll post a reworked class soon to address that.

[gardhr]

Okay, so this does seem to be a better design, avoiding the whole int/bool ambiguity altogether:

/*
	A possible candidate for propagating state-integrity from functions. 
	Note: the const-ness of rvalues of this class is NOT preserved 
	(necessary to make everything work properly, unfortunately)
*/
class error
{
	public:
		
	enum 
	{ 
	/*
		Give a few of options for the most common naming choices
	*/
		success, 
		good = success, 
		failure, 
		fail = failure, 
		bad = fail, 
	/*
		User-defined codes should start at error::define
	*/
		define 
	};
	
	error(int code = success, bool throws = true)
	{
		(*this)(code, throws);
	}
	
	error(const error& other)
	{
		*this = other;
	}	
	
	error& operator () (int code, bool throws = true)
	{
		this->code = code;
		this->throws = throws;
		return *this;
	}

	operator int ()
	{
		throws = false;
		return code;
	}
	
	bool operator == (const error& other)
	{
	/*
		We'll use this type of comparison in the event that 
		derived classes decide to use bitmasks for error codes
	*/
		return code & other.code;
	}
	
	bool operator != (const error& other)
	{
		return !(*this == other);
	}	
	
	error& operator = (const error& other)
	{
		code = other.code;
		throws = true;
		const_cast<error&>(other).throws = false;
		return *this;
	}	
	
	virtual ~error()
	{
		if(code != success && throws)
			throw error(code, false);
	}		
	
	protected:
		
	int code;
	bool throws;
};

/*
	Example usage:
*/
#include <iostream>

class example
{
	public:
		
	error broken()
	{
	/*
		Note: a 'true' value, in this sense, means that there was in fact an error
	*/
		return true;
	/*
		Alternately:
	*/
		return error::fail;
	}

	static void report(int pos, int code = error::fail)
	{
		std::cout << "(" << pos << "): " << (code == error::success ? "success" : "fail") << std::endl;
	}	
};

int main(void)
{
	example test;
	try
	{
	/*
		Status NOT checked, so exception is thrown (but eventually caught)
	*/
		test.broken();
	}
	catch(error&)
	{
		example::report(1);
	}
/*
	If 'state' is never checked by the time it goes out of scope, an exception will be thrown
*/
	error state = test.broken();
/*
	Okay, 'state' is checked, so NO exception is thrown
*/
	example::report(2, state);
/*
	Status checked, so NO exception is thrown
*/
	example::report(3, test.broken());
/*
	Status NOT checked, so exception is thrown (but not caught, so program terminates)
*/
	test.broken();
/*
	We never get this far, of course
*/
	example::report(4);
}

Edit: Removed public access to the error code, as it could have lead to some unpleasant surprises (ie: checking the code directly wouldn't disable exception propagation!).

[kmdv]

I will start with this one:

[code]
enum
{
success,
good = success,
failure,
fail = failure,
bad = fail,
}
[code]

What the hell is that? There are only two possibilities: it succeeded or it failed. It should be a boolean.

	error& operator () (int code, bool throws = true)
	{
		this->code = code;
		this->throws = throws;
		return *this;
	}

Errors aren't functions, they are objects. Invoking the () operator on error instances is confusing. It is not a constructor or an assignment operator.

	operator int ()
	{
		throws = false;
		return code;
	}

Error prone. I would use a getter. This is just an ugly way of avoiding a getter.

	error& operator = (const error& other)
	{
		code = other.code;
		throws = true;
		const_cast<error&>(other).throws = false;
		return *this;
	}

The const_cast may produce an undefined behaviour. You should never change state of the object being copied.

	virtual ~error()
	{
		if(code != success && throws)
			throw error(code, false);
	}

Destructors are NOT allowed to throw exceptions!

Removing the public member variable was a good move.

[gardhr]

I've made some more changes (added a public member function for checking the error code, renamed member variables, and streamlined a bit), but wasn't able to edit the post, so here's the current version:

/*
	A possible candidate for propagating state-integrity from functions. 
	Note: the const-ness of rvalues of this class is NOT preserved 
	(necessary to make everything work properly, unfortunately)
*/
class error
{
	public:
		
	enum 
	{ 
	/*
		Give a few of options for the most common naming choices
	*/
		success, 
		good = success, 
		failure, 
		fail = failure, 
		bad = fail, 
	/*
		User-defined codes should start at error::define
	*/
		define 
	};
	
	error(int code = success, bool throws = true)
	{
		(*this)(code, throws);
	}
	
	error(const error& other)
	{
		*this = other;
	}	
	
	error& operator () (int code, bool throws = true)
	{
		m_code_ = code;
		m_throws_ = throws;
		return *this;
	}
	
	error& operator = (const error& other)
	{
		const_cast<error&>(other).m_throws_ = false;
		return (*this)(other.m_code_, true);
	}

	error& operator = (int code)
	{
		return (*this)(code);
	}	

	int code()
	{
	/*
		Checking the code disables exception propagation
	*/
		m_throws_ = false;
		return m_code_;
	}	
	
	operator int ()
	{
		return code();
	}		
	
	bool operator == (const error& other)
	{
	/*
		We'll use this type of comparison in the event that 
		derived classes decide to use bitmasks for error codes
	*/
		return m_code_ & other.m_code_;
	}
	
	bool operator != (const error& other)
	{
		return !(*this == other);
	}	
	
	virtual ~error()
	{
		if(m_code_ != success && m_throws_)
			throw error(m_code_, false);
	}		
	
	protected:
		
	int m_code_;
	bool m_throws_;
};

/*
	Example usage:
*/
#include <iostream>

class example
{
	public:
		
	error broken()
	{
	/*
		Note: a 'true' value, in this sense, means that there was in fact an error
	*/
		return true;
	/*
		Alternately:
	*/
		return error::fail;
	}

	static void report(int pos, int code = error::fail)
	{
		std::cout << "(" << pos << "): " << (code == error::success ? "success" : "fail") << std::endl;
	}	
};

int main(void)
{
	example test;
	try
	{
	/*
		Status NOT checked, so exception is thrown (but eventually caught)
	*/
		test.broken();
	}
	catch(error&)
	{
		example::report(1);
	}
/*
	If 'state' is never checked by the time it goes out of scope, an exception will be thrown
*/
	error state = test.broken();
/*
	Okay, 'state' is checked, so NO exception is thrown
*/
	example::report(2, state);
/*
	Status checked, so NO exception is thrown
*/
	example::report(3, test.broken());
/*
	Status NOT checked, so exception is thrown (but not caught, so program terminates)
*/
	test.broken();
/*
	We never get this far, of course
*/
	example::report(4);
}

[kmdv]

	static void report(int pos, int code = error::fail)
	{
		std::cout << "(" << pos << "): " << (code == error::success ? "success" : "fail") << std::endl;
	}

It would be desirable to add a non-static member function (to the error class) taking a reference to std:stream or returning a string (like exceptions from the standard library do)

	try
	{
		...
	}
	catch(error&)
	{
		example::report(1);
	}

I am wondering what you are trying to catch here. Wouldn't it be better to catch this error by const-reference and use the mentioned non-static member function to print the message?

	error state = test.broken();
	...
	test.broken();

I got lost on that one.

[phantomotap]

O_o

This idea has been done so many times; it always fails for exactly "Destructors are NOT allowed to throw exceptions!" reasons.

If another such object is to be destructed as part of unwinding due to your class being poorly destructed (via exception) that object will ultimately, on almost every implementation, call `std::abort'.

You should log an event an call `std::abort' yourself; at least that doesn't violate the rule.

Also, the way you are going about trying to maintain "const correctness" will never work without your bad use `const_cast'. The source below illustrates how "const correctness" can be maintained by a proxy. I was in a hurry, so it may not compile, but you get the idea.

Soma

#include <cstdlib>

struct ForcedCheck
{
    class ForcedCheckAdapter
    {
        friend class ForcedCheck;
        ForcedCheckAdapter():
            mSuccess(false)
        {
        }
        ForcedCheckAdapter
        (
            bool fSuccess
        ):
            mSuccess(fSuccess)
        {
        }
        bool mSuccess;
    };
    ForcedCheck():
        mSuccess(false)
      , mRaiseError(true)
    {
    }
    ForcedCheck
    (
        ForcedCheck & fRHS // Yes, this is supposed to be non-constant reference.
    ):
        mSuccess(fRHS.driveSuccess())
      , mRaiseError(fRHS.driveThrow())
    {
    }
    ForcedCheck
    (
        bool fSuccess
    ):
        mSuccess(fSuccess)
      , mRaiseError(true)
    {
    }
    ForcedCheck
    (
        const ForcedCheckAdapter & fRHS
    ):
        mSuccess(fRHS.mSuccess)
      , mRaiseError(true)
    {
    }
    ~ForcedCheck()
    {
        doRaiseError();
    }
    ForcedCheck & operator =
    (
        ForcedCheck & fRHS
    )
    {
        doRaiseError();
        mSuccess = fRHS.driveSuccess();
        mRaiseError = fRHS.driveThrow();
    }
    bool driveSuccess()
    {
        bool lSuccess(mSuccess);
        mSuccess = false;
        return(lSuccess);
    }
    bool driveThrow()
    {
        bool lThrow(mRaiseError);
        lThrow = false;
        return(lThrow);
    }
    operator ForcedCheckAdapter()
    {
        mRaiseError = false;
        return(ForcedCheckAdapter(driveSuccess()));
    }
    void doRaiseError()
    {
        if(mRaiseError)
        {
            // Pick one for this example.
            //std::abort();
            //throw("Whatever!");
        }
    }
    bool wasSuccessful()
    {
        mRaiseError = false;
        return(mSuccess);
    }
    bool mSuccess;
    bool mRaiseError;
};

ForcedCheck Test
(
    bool fSuccess
)
{
    return(ForcedCheck(fSuccess));
}

#include <iostream>

int main()
{
    using namespace std;
    try
    {
        Test(true);
    }
    catch(...)
    {
        cout << "Test1: Good!(" << '?' << ")\n";
    }
    ForcedCheck lState(Test(true));
    cout << "Test2: Good!(" << lState.wasSuccessful() << ")\n";
    Test(true);
    cout << "Test2: Good!(" << '?' << ")\n";
    return(0);
}

[gardhr]

What the hell is that? There are only two possibilities: it succeeded or it failed. It should be a boolean.

The user may want to add more error codes, though, and so in this case it makes sense to have success == 0 (similar to the way main() returns zero upon success on most platforms).

Errors aren't functions, they are objects. Invoking the () operator on error instances is confusing. It is not a constructor or an assignment operator.

This seems to be a very common issue of contention amongst C++ developers, but it really just boils down to a matter of taste, I think. That said, a named member function is probably a good idea too.

Error prone. I would use a getter. This is just an ugly way of avoiding a getter.

Yes, I have thought about the possibility of problems arising from the use of implicit conversion, but with the removal of the int/bool ambiguity I think that these concerns have been mitigated. I'd welcome an example use cases proving otherwise, of course. Either way, I already added a public function for that, in anticipation of such objections.

The const_cast may produce an undefined behaviour. You should never change state of the object being copied.

As a general rule, I completely agree with you. But in this case it was the only practical way to get things to work. Maybe a better approach would be to use a shared handle of some sort; checking any instances related by assignment would effectually disable the propagation of an exception.

Destructors are NOT allowed to throw exceptions!

An exception thrown from a destructor whilst another exception is being processed will indeed lead to program abort (by default, anyway), but otherwise the behaviour is perfectly well-defined and legal.

Removing the public member variable was a good move.

Thanks.

[gardhr]

It would be desirable to add a non-static member function (to the error class) taking a reference to std:stream or returning a string (like exceptions from the standard library do)

Given that the error class itself (viewed as a base class) provides so few interesting states to report (ie: just success or failure), it almost seems like overkill to me. Why not just allow derived classes to provide such functionality and then throw and catch those instead, if desired?

I am wondering what you are trying to catch here. Wouldn't it be better to catch this error by const-reference and use the mentioned non-static member function to print the message?

In a real-world application, yes. But in this case I was just putting together a simple test harness to demonstrate usage. I do agree that catching by const-reference would have been a better choice, though.

I got lost on that one.

Those are simply example usages of the class. (1) demonstrates that capturing the error code as a class does not alone disable the exception; it has to be "checked". (2) and (3) are examples of checking the error code, which effectively disables any exception propagation. (4) is an example of forgetting to check the error code, leading to an uncaught exception.

[brewbuck]

MAJOR KUDOS to you for coming up with this idea on your own. This is a real technique, albeit a little-known one, in massively parallel C++ programming circles.

I have implemented a similar technique myself, only to discover (after TWO YEARS) it had already been invented.

phantomotap makes the (good) point that throwing an exception from a destructor is normally a bad idea. However I think the criticism is misapplied here.

The point is to find all the places where you are not checking the return code. Not checking the return code is a programming defect. The purpose of the thrown exception is to indicate the defect. In releasable code, these exceptions should never be generated because all return codes are being appropriately checked. The exception is just a debugging tool.

In my implementation, I do not throw an exception when the status expires in an unchecked state -- instead, I make a virtual call to a global UncheckedStatus object, which by default, aborts execution and dumps core.

This is a GREAT IDEA that will SAVE YOUR ASS, as long as you don't think of it as a "lazy error check" and try to get away with catching the exceptions that are generated. The exceptions are indicative of bugs which should be fixed.

Seriously, I can't praise you enough for this one.

EDIT: I did not check your code for correctness. There are some subtleties to implementing this, but you seem to be on the right track.

[brewbuck]

Also, the way you are going about trying to maintain "const correctness" will never work without your bad use `const_cast'. The source below illustrates how "const correctness" can be maintained by a proxy. I was in a hurry, so it may not compile, but you get the idea.

In my implementation, I simply made the flag mutable.

[iMalc]

In my implementation, I simply made the flag mutable.

Damn, I was just going to say that this would warrant m_throws_ being mutable.

[kmdv]

The user may want to add more error codes, though, and so in this case it makes sense to have success == 0 (similar to the way main() returns zero upon success on most platforms).

So don't define it as an enum with predefined "bad" code (multiple names for the same thing just shocked me...).

This seems to be a very common issue of contention amongst C++ developers, but it really just boils down to a matter of taste, I think. That said, a named member function is probably a good idea too.

I consider it a bad usage, not a taste.

Yes, I have thought about the possibility of problems arising from the use of implicit conversion, but with the removal of the int/bool ambiguity I think that these concerns have been mitigated. I'd welcome an example use cases proving otherwise, of course. Either way, I already added a public function for that, in anticipation of such objections.

It's not about technical problems (which could be removed with new usage of the 'explicit' keyword) it is just confusing. For example, implicit conversion to bool for shared pointers is obvious, since C++ allows such for raw pointers.

As a general rule, I completely agree with you. But in this case it was the only practical way to get things to work. Maybe a better approach would be to use a shared handle of some sort; checking any instances related by assignment would effectually disable the propagation of an exception.

It's undefiend behaviour, there is no discussion. And I certainly wouldn't call it a practical way.

Given that the error class itself (viewed as a base class) provides so few interesting states to report (ie: just success or failure), it almost seems like overkill to me. Why not just allow derived classes to provide such functionality and then throw and catch those instead, if desired?

Because you are not going to catch every single kind of exception. Base class should have a virtual getter to allow derieved classes to reimplement their own ones.

An exception thrown from a destructor whilst another exception is being processed will indeed lead to program abort (by default, anyway), but otherwise the behaviour is perfectly well-defined and legal.

Just because it is defined and it compiles does not mean it is good.

Those are simply example usages of the class. (1) demonstrates that capturing the error code as a class does not alone disable the exception; it has to be "checked". (2) and (3) are examples of checking the error code, which effectively disables any exception propagation. (4) is an example of forgetting to check the error code, leading to an uncaught exception.

By that I meant that this usage is very unintuitive.

[brewbuck]

Other cool tricks you can do with this...

Templatize it so it wraps a generic error code (int, enum, whatever), instead of limiting to just bool.

Give the ability to discard an error code deliberately without checking it. You can ignore an error, but you need to call a function to explicitly do it.

Give the ability to detect positive vs. negative status, for instance you can trigger a breakpoint whenever an error is first generated, but continue along quietly when things are going fine.

Make the whole thing controlled by a compile-time flag. When disabled, the return codes all revert back to plain old int/bool/enum again. Now your debug builds can have really sweet error tracing, and your release builds don't suffer from it.

Remember that this is all great for debugging, but not at all suitable for an error code architecture.

[phantomotap]

Forcing the check of a result before using that result is a fine way of tracking down the source of some problems, but this concept only ever half works at best and even then causes false negatives in the face of unrelated but real problems. Again, the issue is that this design can't work to actually do what people always want it to do because raising an exception during unwinding aborts, `std::uncaught_exception()' is basically useless, and only throwing an exception if an exception isn't being processed is retarded.

[Edit]
In shorter words, C++ is awesome, but sometimes real world policies have to be enforced with tools beyond what C++ can provide.

Well, at least until I can convince the commit to add `__forcecheck' at least for native types.
[/Edit]

[Edit]
By the way, I'm not trying to harp on the OP. A lot of great programmers have this idea. If it could be made to work completely without problems like those below it would be awesome.
[/Edit]

Here let me give you a few examples when a "forced check" object uses object state to determine destructor behavior.

If the object always attempts to throw, as a consequence of not being checked, an unrelated exception will result in a false negative.

ForcedCheck lState(DoSomething());
// ...
DoSomethingElse(); // This object may raise an exception during debugging and release builds.
// ...
if(lState)
{
    // We examine the forced check object. It shouldn't be an issue.
    // However, it is a problem because the `ForcedCheck' object
    // hasn't necessarily been updated with this new state because
    // the call to `DoSomethingElse' may throw an exception before
    // we get to this point. Because the state has not changed
    // if `DoSomethingElse' throws an exception the `lState'
    // destructor will try and report that problem by raising
    // an exception which ultimately will trigger an abort.
}

Okay, we have an abort and on most systems that means no information about either exception. Let's update our exception base class to log information during debug builds. With this change in place we get our information about the real exception that caused unwinding as well as information about the unchecked return value even in the case of an abort.

Wait, that doesn't work; a false negative eats into development time without reason. Let's say we force people to examine the object immediately with no intervening code. No, we can't do that within the framework of C++. Okay, we reach for other tools to enforce that policy. Oh, literally any method that can enforce that policy can enforce the check of the return value without needing an object that attempts to enforce a check within the framework of C++.

Okay, let's try to use some method to annotate the information. Let's reach for `std::uncaught_exception()'. We can't blindly ignore the possibility that the check wasn't made, but we can't start littering debug logs with false negatives either. Let's make the log entry, but let's add a little flag that says that the entry occurred as a consequence of unwinding so that the problem is referenced but the developer can focus on the other issue first.

class ForcedCheck
{
    // ...
    ~ForcedCheck()
    {
        DebugLog() << "ForcedCheck";
        if(std::uncaught_exception())
        {
            DebugLog() << "(uncaught exception)";
        }
        else
        {
            DebugLog() << "(failure to examine result)";
        }
        DebugLog() << __LINE__ << ',' << __FUNCTION__ << '\n';
    }
    // ...
};

Let's test drive that bad boy. Wait. It doesn't work on "Popular Compiler Suite". As it turns out, "Popular Compiler Suite" doesn't implement `std::uncaught_exception()' properly; it only works if the exception is a child of `std::exception'. That's no big deal for our code. We can start using that as a base class for all exceptions. That problem was solved easily enough. Now let's try and find a solution for other exceptions.

Let's try using "Other Compiler Suite". It doesn't work under that suite either. It turns out that `std::uncaught_exception()' always returns false regardless of the state of the exception mechanism. With this suite all of our log entries are going to look the same so we have no way of knowing if an entry is a maybe or definite.

Soma