Jump to content

  • Log In with Google      Sign In   
  • Create Account


goto considered harmful within global macros

  • You cannot reply to this topic
21 replies to this topic

#1 swiftcoder   Senior Moderators   -  Reputation: 9761

Like
0Likes
Like

Posted 13 October 2013 - 07:41 AM

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

 

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.


Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


Sponsor:

#2 JTippetts   Moderators   -  Reputation: 8259

Like
0Likes
Like

Posted 13 October 2013 - 07:52 AM

Yikes.



#3 phantom   Moderators   -  Reputation: 6900

Like
0Likes
Like

Posted 13 October 2013 - 08:23 AM

Dear god o.O

#4 l0calh05t   Members   -  Reputation: 671

Like
0Likes
Like

Posted 13 October 2013 - 08:35 AM

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, 13 October 2013 - 08:38 AM.


#5 Cornstalks   Crossbones+   -  Reputation: 6974

Like
2Likes
Like

Posted 13 October 2013 - 09:51 AM

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.


[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#6 SiCrane   Moderators   -  Reputation: 9496

Like
7Likes
Like

Posted 13 October 2013 - 09:54 AM

Really the thing that gets me most about this macro is that the name isn't in all caps. That's just wrong.



#7 Krohm   Crossbones+   -  Reputation: 3017

Like
1Likes
Like

Posted 14 October 2013 - 12:53 AM

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



#8 Hodgman   Moderators   -  Reputation: 28604

Like
3Likes
Like

Posted 14 October 2013 - 01:12 AM

The real WTF is that other half of the error handling system isn't hidden behind an equivalent macro laugh.png tongue.png
#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, 14 October 2013 - 08:05 PM.


#9 Migi0027   Crossbones+   -  Reputation: 1519

Like
0Likes
Like

Posted 14 October 2013 - 03:40 PM

 

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

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

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

 

WTF Indeed blink.png .



#10 Servant of the Lord   Crossbones+   -  Reputation: 18144

Like
0Likes
Like

Posted 16 October 2013 - 05:33 PM

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... blink.png


It's perfectly fine to abbreviate my username to 'Servant' rather than copy+pasting it all the time.

[Fly with me on Twitter] [Google+] [My broken website]

All glory be to the Man at the right hand... On David's throne the King will reign, and the Government will rest upon His shoulders. All the earth will see the salvation of God.                                                                                                                                                            [Need web hosting? I personally like A Small Orange]
Of Stranger Flames - [indie turn-based rpg set in a para-historical French colony] | Indie RPG development journal


#11 Paradigm Shifter   Crossbones+   -  Reputation: 5231

Like
0Likes
Like

Posted 16 October 2013 - 05:45 PM

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.


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#12 swiftcoder   Senior Moderators   -  Reputation: 9761

Like
0Likes
Like

Posted 16 October 2013 - 08:48 PM


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.


Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#13 Paradigm Shifter   Crossbones+   -  Reputation: 5231

Like
0Likes
Like

Posted 16 October 2013 - 08:59 PM

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, 16 October 2013 - 09:03 PM.

"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#14 swiftcoder   Senior Moderators   -  Reputation: 9761

Like
0Likes
Like

Posted 16 October 2013 - 09:02 PM


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 blink.png


Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#15 Paradigm Shifter   Crossbones+   -  Reputation: 5231

Like
3Likes
Like

Posted 16 October 2013 - 09:17 PM

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.


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#16 wodinoneeye   Members   -  Reputation: 721

Like
0Likes
Like

Posted 24 October 2013 - 06:17 AM

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

 

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


--------------------------------------------Ratings are Opinion, not Fact

#17 Brother Bob   Moderators   -  Reputation: 7903

Like
1Likes
Like

Posted 24 October 2013 - 06:44 AM

 

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

 

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...



#18 ultramailman   Prime Members   -  Reputation: 1561

Like
0Likes
Like

Posted 25 October 2013 - 11:51 AM

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

 

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



#19 swiftcoder   Senior Moderators   -  Reputation: 9761

Like
0Likes
Like

Posted 25 October 2013 - 12:26 PM

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.

Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#20 fastcall22   Crossbones+   -  Reputation: 4149

Like
3Likes
Like

Posted 25 October 2013 - 12:59 PM

TRTWTF is nobody escaped expression in parenthesis:
 
#define CheckSuccess(condition) do { \
    if (!(condition)) { \
        goto onError; \
    } \
} while (0) 
Hurr durr Ph'nglui mglw'nafh Cthulhu CheckSuccess(obj.frobnicate() || obj.bang()) R'lyeh wgah'nagl fhtagn and what not...

Edited by fastcall22, 25 October 2013 - 01:01 PM.

c3RhdGljIGNoYXIgeW91cl9tb21bMVVMTCA8PCA2NF07CnNwcmludGYoeW91cl9tb20sICJpcyBmYXQiKTs=





PARTNERS