goto considered harmful within global macros

This topic is 1538 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

I was really quite surprised when I first attempted to add error checking at work, and my compilation failed with a [tt]error: use of undeclared label 'onError'[/tt].

From a ubiquitous global header in a (thankfully recently deceased) library within our project:

#define CheckSuccess(condition) do { \
if (!condition) { \
goto onError; \
} \
} while (0) 

The idea that an error checking macro would require the current function to implement a specifically named label for error cleanup (or, indeed, that there would be significant error cleanup not handled by RAII), is a little unpleasant to me...

I should probably add that the author of this particular gem thought that they were working around the lack of exception handling on our target platform.

Yikes.

Dear god o.O

Share on other sites

Meh, macros like this are common in C programs. Also popular:

#define CHECK(x) do { if(err = (x)) goto error; } while(0)

to save the actual error code (and assuming 0 = no error), which requires a variable of the appropriate type in addition to the proper goto label.

It should probably not be defined in a global header though.

Edited by l0calh05t

Share on other sites

I'm 100% surprised a velociraptor attack did not ensue. That macro only saves you from typing 7 keystrokes too... totally not worth using it IMO. And I hate macros like that for debugging and stepping through code.

Also, condition really should've been in parentheses (like in l0calh05t's post), because other wise things like CheckSuccess(false || true) would've resulted in madness.

Share on other sites

I feel something in my bowels. I'm going AFK a few minutes.

Share on other sites
The real WTF is that other half of the error handling system isn't hidden behind an equivalent macro
#define CheckSuccess(condition) do { \
if (!condition) { \
goto onError; \
} \
} while (0)

#define OnError do { if(false) { \
onError: (void)0
#define EndError }} while (0) 
I mean, come on, if you're going to abstract away half of your error handling system behind your own invented keyword, at least be consistent and apply the same abstraction to the other half of the system too! Edited by Hodgman

Share on other sites

The real WTF is that other half of the error handling system isn't hidden behind an equivalent macro

#define CheckSuccess(condition) do { \
if (!condition) { \
goto onError; \
} \
} while (0)

#define OnError do { if(false) { \
onError: (void)0
#define EndError }} while (0) 

WTF Indeed  .

Share on other sites

On the rare occasions I need macros that are specific to a function or small set of related functions, I define them in the source file and then #undef them afterward. I've yet to put a goto in one though...

Share on other sites

Pfft, I once ended a macro with an "else" (and nothing afterwards).

It was my first job though so I didn't know much better.

I had a load of check for joypad presses all over the code (bad I know). Then we got the standards guidelines through and pressing select and start for 3 seconds had to return to the menu. So I hacked it with a dodgy macro ;) That was when I used longjmp to return from anywhere back to the main menu too.

Share on other sites

That was when I used longjmp to return from anywhere back to the main menu too.

That's deliciously evil, but it's worth keeping in mind that it isn't unprecedented.

Your longjmp is effectively just implementing a call/cc, and would be considered a (fairly) sane method of control flow in most functional languages.

Share on other sites

It's pretty similar to using exceptions (it resets the stack and jumps to the call to setjmp), without calling any destructors. It was C anyway, so that didn't matter.

Structured exception handling in Win32 uses setjmp/longjmp (but hides them behind macros).

EDIT: My evil macro was basically something which went before a call to GetPadState, something like this in pseudocode

macro CHECK_EXIT_GAME:

if(select and start held down for 3 seconds) longjmp MainMenuHandler

else

end macro

;)

Edited by Paradigm Shifter

Share on other sites

It's pretty similar to using exceptions (it resets the stack and jumps to the call to setjmp)

Aye, but "control flow via exceptions" gives everyone the heebee jeebees.

call/cc only typically causes heart attacks in procedural programmers

Share on other sites

The funniest thing that happened in that project was when I had several bugs active at the same time:

I was running he NTSC version on a UK telly (so it was in black and white, TVs these days can handle NTSC and PAL). Not really a bug

There was a bug where the main menu music kept restarting after every data read

After a data read the CD speed was 2x so the music was way fast and high pitched (it was annoying steel drum band music as well!)

It was a golf game but something was wrong with the AI aiming so it was hitting the ball backwards instead of forwards

End result it was like watching a silent B&W golf comedy with mad double speed steel band music where the AI golfer kept hitting the ball backwards towards the camera.

Share on other sites

I was really quite surprised when I first attempted to add error checking at work, and my compilation failed with a [tt]error: use of undeclared label 'onError'[/tt].

From a ubiquitous global header in a (thankfully recently deceased) library within our project:

#define CheckSuccess(condition) do { \
if (!condition) { \
goto onError; \
} \
} while (0) 

The idea that an error checking macro would require the current function to implement a specifically named label for error cleanup (or, indeed, that there would be significant error cleanup not handled by RAII), is a little unpleasant to me...

I should probably add that the author of this particular gem thought that they were working around the lack of exception handling on our target platform.

I used to do it simpler...

#define CheckPrecondition(condition)  {  if ( ! condition) goto  ON_PRECONDITION_FAIL; }

Could have sworn I used to get that to work in a (native code) FSM macro simplified script language

Share on other sites

I was really quite surprised when I first attempted to add error checking at work, and my compilation failed with a [tt]error: use of undeclared label 'onError'[/tt].

From a ubiquitous global header in a (thankfully recently deceased) library within our project:

#define CheckSuccess(condition) do { \
if (!condition) { \
goto onError; \
} \
} while (0) 

The idea that an error checking macro would require the current function to implement a specifically named label for error cleanup (or, indeed, that there would be significant error cleanup not handled by RAII), is a little unpleasant to me...

I should probably add that the author of this particular gem thought that they were working around the lack of exception handling on our target platform.

I used to do it simpler...

#define CheckPrecondition(condition)  {  if ( ! condition) goto  ON_PRECONDITION_FAIL; }

Could have sworn I used to get that to work in a (native code) FSM macro simplified script language

It is simpler but also prone to errors that the do-while(false) method is isn't. The do-while(false) method requires a semicolon to work, while it is optional and can silently change the behavior of your code whether it is there or not with your variant.

But, I'd say that such hacks deserves some happy debugging times when they do change the behavior...

Share on other sites

http://www.chiark.greenend.org.uk/~sgtatham/coroutines.html

Not exactly goto in macro, but switch case is similar to goto.

Please refer to the gem at the bottom of that page:
* PuTTY is a Win32 Telnet and SSH client. The SSH protocol code contains real-life use of this coroutine trick.
As far as I know, this is the worst piece of C hackery ever seen in serious production code.

Share on other sites
TRTWTF is nobody escaped [tt]expression[/tt] in parenthesis:

#define CheckSuccess(condition) do { \
if (!(condition)) { \
goto onError; \
} \
} while (0)

Hurr durr Ph'nglui mglw'nafh Cthulhu [tt]CheckSuccess(obj.frobnicate() || obj.bang())[/tt] R'lyeh wgah'nagl fhtagn and what not... Edited by fastcall22

Share on other sites

It is simpler but also prone to errors that the do-while(false) method is isn't. The do-while(false) method requires a semicolon to work, while it is optional and can silently change the behavior of your code whether it is there or not with your variant.

I used to do it simpler...

#define CheckPrecondition(condition)  {  if ( ! condition) goto  ON_PRECONDITION_FAIL; }

Could have sworn I used to get that to work in a (native code) FSM macro simplified script language

But, I'd say that such hacks deserves some happy debugging times when they do change the behavior...

It was used within an all 'scripted' code block setting (FSMs for object behaviors)  where the rest of the mini-language macros were used of similar definitions (and were not interwoven with normal code).  That language statement  defines actually would look more like  :

#define PRECOND(CONDX)  {  if ( ! CONDX) goto  STATEX##PRECOND_FAIL; }

The more interesting  defines were for the FSM state wrappers to work within  a BIG SWITCH.   and the ones needed to break each behavior state into 3 seperately run  behavioral phases of the game loop.  There could be  competing tasks for each object state,  which required seperated out phase processing (by tasks and individual target factors)  : first - discovery via state-task specialized sensor searches and situation conditions, then second - a phase prioritizing the search results , picking a 'best' task and executing the corresponding actions for the task,   third  -  a  epilog phase which did evaluations and state transitions based on results.

Much of the overall complexity came from having interacting objects be frozen for equal evaluation, and after priority selection  both objects being locked into an interaction mode (locking out other object's interactions and forcing wait time for the duration of the action (related to the animation time).

The whole thing was organized to be table form programming (but was compiled into native code).  It had shortcomings of only being able to handle rudimentary inter-object interactions and signalling before becoming cumbersome.  I had to shift more complex object AI  operations to a Planner type scheme (easier to handle interupted tasks when you just abandon and reevaluate everything and restart).  The original mechanism was retained for objects with  simple reactionary and single minded behaviors as it was more lightweight and was still used on a majority of objects in the simulation.

The script code using such macro statments were not nested (eliminated most of any problematic cases) and quite regular in the way they were used (and so easy to spot any syntax mistakes).    So there were few problems of that type.

The debugging fun was actually in the intricacies of the inter-object behavior interactions with follow-on sequential states for many tasks (I was considering building a wizard to help streamline the creation of the script 'code' data to handle alot of the table building 'bookkeeping'....)

Edited by wodinoneeye