RE: general question..

[Stefan Dösinger]

Can you give some examples of such code? Nobody here is a goto-maniac, but
in all cases I know the goto is used for a good reason. Most of the time it
is used for error path to avoid ugly if() nesting and/or code duplication
when freeing partially allocated objects
> -----Original Message-----
> From: [e-mail] [mailto:wine-patches-
> [e-mail]] On Behalf Of Chris Ahrendt
> Sent: Monday, July 21, 2008 8:42 PM
> To: [e-mail]
> Subject: general question..
>
> As I have been going through trying to debug the everquest2 issues on
> my
> machine I have run into a few places where I think the code should be
> changed a little ( this alot of times is where there is a GOTO in the
> code). My question is in the case where I do find these goto's and I
> rewrite the code  should I submit the changes here for approval?
> I have been doing some code reviews and found a few places where
> there are goto's that don't need to be there. I guess I am old school
> and really dislike code with goto's (they make debugging and
> maintenance
> a nightmare (I used to maintain 20 year old cobol code  along time ago
> =) )
>
> Chris

general question..

[Chris Ahrendt]

Stefan Dösinger wrote:
> Can you give some examples of such code? Nobody here is a goto-maniac, but
> in all cases I know the goto is used for a good reason. Most of the time it
> is used for error path to avoid ugly if() nesting and/or code duplication
> when freeing partially allocated objects
> > -----Original Message-----
> > From: [e-mail] [mailto:wine-patches-
> > [e-mail]] On Behalf Of Chris Ahrendt
> > Sent: Monday, July 21, 2008 8:42 PM
> > To: [e-mail]
> > Subject: general question..
> >
> > As I have been going through trying to debug the everquest2 issues on
> > my
> > machine I have run into a few places where I think the code should be
> > changed a little ( this alot of times is where there is a GOTO in the
> > code). My question is in the case where I do find these goto's and I
> > rewrite the code  should I submit the changes here for approval?
> > I have been doing some code reviews and found a few places where
> > there are goto's that don't need to be there. I guess I am old school
> > and really dislike code with goto's (they make debugging and
> > maintenance
> > a nightmare (I used to maintain 20 year old cobol code  along time ago
> > =) )
> >
> > Chris

Sure =)

Well here is my list so far :

device.c - filled with goto's if you need the routines I can supply 
them... they can be moved to a routine and called...

context.c - This case its just a return with noting else.. why do a goto 
why not just do the return?

directx.c  - same... this can be moved to a routine and simplified..
this is in routine WineD3D_CreateFakeGLContext which I was tracing down 
some stuff...

pixelshader.c - why not make this into a routine?

provider.c - same

These are just the ones I have run accross so far..

In the case of code dupes instead of using a goto like the 
createfakeGLContext why not make the

fail:
     if(wined3d_fake_gl_context_hdc)
         ReleaseDC(wined3d_fake_gl_context_hwnd, 
wined3d_fake_gl_context_hdc);
     wined3d_fake_gl_context_hdc = NULL;
     if(wined3d_fake_gl_context_hwnd)
         DestroyWindow(wined3d_fake_gl_context_hwnd);
     wined3d_fake_gl_context_hwnd = NULL;
     if(glCtx) pwglDeleteContext(glCtx);
     LeaveCriticalSection(&wined3d_fake_gl_context_cs);
     return FALSE;

into a routine and then do a

return fakeContextFail(glCtx);

Then of course above createfakeGLContext define
fakeContextFail(......)

Not saying Goto's are completely bad.. in some cases they are good for 
avoiding some tricky situations but alot of times they are not... just 
my $.02 not trying to start a flame war guys just trying to help =)

Chris

general question..

[Vitaliy Margolen]

Chris Ahrendt wrote:
> fail:
>      if(wined3d_fake_gl_context_hdc)
>          ReleaseDC(wined3d_fake_gl_context_hwnd, 
> wined3d_fake_gl_context_hdc);
>      wined3d_fake_gl_context_hdc = NULL;
>      if(wined3d_fake_gl_context_hwnd)
>          DestroyWindow(wined3d_fake_gl_context_hwnd);
>      wined3d_fake_gl_context_hwnd = NULL;
>      if(glCtx) pwglDeleteContext(glCtx);
>      LeaveCriticalSection(&wined3d_fake_gl_context_cs);
>      return FALSE;
>
> into a routine and then do a
>
> return fakeContextFail(glCtx);

That will turn into nightmare when tracking a locking problem down. Call to 
LeaveCriticalSection should always be in the same function as 
EnterCriticalSection. Same applies to any resource that you do not want to 
leak. That is much more important then calling a function a "routine" and 
replacing all goto's with that function call.

Also I'd like to show how you can free local variables?

general question..

[Chris Ahrendt]

Vitaliy Margolen wrote:
> Chris Ahrendt wrote:
> > fail:
> >      if(wined3d_fake_gl_context_hdc)
> >          ReleaseDC(wined3d_fake_gl_context_hwnd, 
> > wined3d_fake_gl_context_hdc);
> >      wined3d_fake_gl_context_hdc = NULL;
> >      if(wined3d_fake_gl_context_hwnd)
> >          DestroyWindow(wined3d_fake_gl_context_hwnd);
> >      wined3d_fake_gl_context_hwnd = NULL;
> >      if(glCtx) pwglDeleteContext(glCtx);
> >      LeaveCriticalSection(&wined3d_fake_gl_context_cs);
> >      return FALSE;
> >
> > into a routine and then do a
> >
> > return fakeContextFail(glCtx);
>
> That will turn into nightmare when tracking a locking problem down. Call 
> to LeaveCriticalSection should always be in the same function as 
> EnterCriticalSection. Same applies to any resource that you do not want 
> to leak. That is much more important then calling a function a "routine" 
> and replacing all goto's with that function call.
>
> Also I'd like to show how you can free local variables?

ok it was late when I was writing this =)

the glCtx and the leave critical section can be moved back into the main 
routine.. the hdc and hwnd are globals so thats not a problem...
next question is does the hdc and hwnd memory allocation get freed in 
the releaseDC and destroy window calls before the nulls?

Chris