error handling proposal

Editor
Mar 16, 2012 at 3:29 PM
Edited Mar 16, 2012 at 3:34 PM

As I'm playing with Hieroglyph I'm noticing this kind of boilerplate cropping up (e.g.):

	int iRasterizerState = m_pRenderer11->CreateRasterizerState( &rsConfig );
	if ( iRasterizerState == -1 ) {
		Log::Get().Write( L"Failed to create light rasterizer state" );
		assert(false);
	}

This is somewhat verbose and I can't think of many circumstances under which these errors are recoverable anyway.

So instead might we consider something like the following (just sketching it out on the fly here)?

First add a trivial check method - something like this:

    inline bool Failed(HRESULT hr, string log)
    {
        if (FAILED(hr)) {
	    Log::Get().Write( log ); // NB: would be better to log the actual hr result code
        #ifdef USE_EXCEPTION
            throw std::exception();
	#else
	    return true;	
	#endif
        }
	else
           return false;
    }

Then where methods have checks like this (e.g.):

	if ( FAILED( hr ) )
	{
		Log::Get().Write( L"Failed to create rasterizer state!" );
		return( -1 );
	}

Change them to this (e.g.):

	if (Failed(hr, L"Failed to create rasterizer state!")) return -1;

This way lazy people (like me) can just enable exceptions and whatever happens, happens. And if others still want to explicitly check results then that's fine too. And of course the error checks become  a bit more compact in the engine itself.

Worthwhile?

- JK

Mar 16, 2012 at 6:06 PM

In my opinion, it is an important design choice to select error handling using classical method (like Hieroglyph currently use) or using exception.

My development workhorse is not C++ but Delphi. And in Delphi, I almost always use exception to handle errors. This mostly simplifies the development because you can put the error handling at the place where it is best. Traditional method impose to have error handling at each level which is tedious and error prone. So I vote for exception handling.

But wait ! to be efficient, this must be applyed everywhere in the library. Mixing both modes is not good design.

My 2c.

 

Mar 16, 2012 at 6:35 PM

Exceptions should never be used in game code, especially in core engine code, because in C++ they cause big performance hit - no sane C++ engine developer would use them, at least, as I said, not in engine code. Handling errors should generate log message and then, depending whether it's something serious or not, clean and shutdown everything or just do nothing else - generally debug builds should always crash exit with message, while release builds should try to avoid this as much as possible (kind of similar to asserts in code).

Mar 16, 2012 at 8:11 PM

Are you sure ? The normal flow should not trigger exception. Only errors should. And errors should be the... exception !

Read this: www dot nwcpp dot org slash Downloads/2006/ehc.ppt

 

Mar 16, 2012 at 9:04 PM
Edited Mar 16, 2012 at 9:07 PM

Yes, the normal flow in typical application would be to throw exception, however, game engines are very specific piece of software and every respectable c++ engine programmer should avoid them - usually exceptions are completely turned off in compiler for engine code, it's standard even (or rather, especially) in studios that develop AAA games and/or technology.

Also, sorry, but right now I don't have any kind of office suite to open this presentation, but I will check it as soon as I will get one! :)

Editor
Mar 16, 2012 at 9:12 PM
Edited Mar 16, 2012 at 9:20 PM

Indeed, I'm always concerned about performance - but I would note for the case of resource creation this should be a non-issue, as, for performance reasons, any sane engine will create all resources up-front and never touch them again anyway (modify sure, create no). But in any event, this is why I suggested a macro to allow exceptions to be used -- or not -- as the user so chooses. For demo/tutorial code it's convenient to hide the explicit error checks as it significantly improves the signal:noise ratio. Note I would also be fine with a simple assert instead of an exception throw. As long as it's not in my face ;-)

Mar 16, 2012 at 10:50 PM

Well, to be honest I was referring to fpiette's proposition of using exceptions everywhere - your approach seems much better to me, although I would modify it so it doesn't throw exception, but use assert instead. ;)

Coordinator
Mar 17, 2012 at 3:55 AM

I can give my opinion, and I would like to keep the discussion going.  The engine was originally started when I didn't really have an opinion on this topic, which was over 10 years ago (when I was just starting out in C++).  At the time, I was learning from lots of D3D sample programs back around the D3D8/9 days - when most of the error handling was always error code checking.  Since I had started out with this style, I just stuck with it throughout the development since then. 

To be perfectly honest, there are places in the code base that are a little loose with the error handling - so this is a good topic to consider so that we can have a standardized way to move forward.  I agree with Francois on that point - it should be all or nothing, regardless of the method.

My current thoughts on this are actually a hybrid of the two main proposals.  I have an Event defined for error messages that is used in a couple places in the renderer (in the load shader method especially).  This is the EvtErrorMessage class, and it allows the error message to be passed to the event system.  All of the sample applications register for this message, and then can handle it as each application sees fit.  In this way, you can crash hard if you want, or you can produce a log message and try to continue on, or whatever else.  You could even throw an exception in the application handler if you want to...

Using this mechanism throughout the code base could be simplified with a method similar to what jmkinzer mentioned - a type of helper function that can quickly indicate an 'error', and then let the application handle it accordingly.  What do you guys think about this approach?  I think it keeps things flexible, but still addresses the cleanliness issue.

Editor
Mar 17, 2012 at 6:15 AM

Wow, it's been a long journey then :-)

Sounds great! Yeah, my strawman should have been clearer but my thinking was in the same direction as yours - i.e. the key is simply to centralize error handling and then if you want be agnostic w.r.t. exact failure mechanism such that people can plugin the handler they want (asserts, exceptions, ...).

Mar 17, 2012 at 10:16 AM

Using an event to handle error is only part of the work. the event is handy to have a centralized place where to do logging or alerting. But really handling the error has - most of the time - to be done closer to where the error happened (for example to release resources or clean what has to be cleaned).

One very important design consideration is that exceptions should be thrown only in case of error. Throwing exception for normal processing is defenitely a bad design and this is where exceptin handling starts to take time. An example of such bad design is the following I already saw : the developer used exception to test if a number was integer or floting: he first try with an integer only function and then if an exception is thrown, he switch to floating point handling. This is exactly what should NEVER be done. If you keep away from such design, then performance is almost unchanged or even better because there are less tests executed for nothing in 99.999% of the time.

 

Coordinator
Mar 17, 2012 at 12:43 PM

I agree with what you are saying about exceptions - in cases where something is truly so wrong that the engine can't go on, then we should have an exception.  However, lots of people don't want to use exception handling (which is a valid design choice) and I don't want the API to force them to use it.  So I like the idea of allowing the application to choose if they want to use exceptions from within the event handler - this also allows many different objects to be able to handle the event too, like a simple logger, the app object, an email system or whatever.

This has been a great discussion - thanks to everyone for bringing their opinions to the table!