Re: general question..
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
RE: general question..
The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Well, in ARB shader constants are a context global state, opposed to GLSL where they are a per-shader property. Thus the constant tracking for ARB is moved to the context rather than in the shader. It could maybe be made a shader backend callback, currently it is controlled by a flag in the shader backend
Re: general question..
On Mon, Jul 21, 2008 at 8:41 PM, Chris Ahrendt [EMAIL PROTECTED] wrote: 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 =) ) You're not going to find many in this project that agree with your stance on the use of goto's. That being said, I'm curious to see where you think the use of goto's should be replaced by something else. Feel free to send a RFC to wine-devel. P.S. Please don't send discussion emails to wine-patches. wine-devel is for discussion and wine-patches is for patches only. -- James Hawkins
RE: general question..
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: [EMAIL PROTECTED] [mailto:wine-patches- [EMAIL PROTECTED] On Behalf Of Chris Ahrendt Sent: Monday, July 21, 2008 8:42 PM To: [EMAIL PROTECTED] 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
Re: general question..
On Mon, Jul 21, 2008 at 9:24 PM, Chris Ahrendt [EMAIL PROTECTED] wrote: 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... wine$ find . -name device.c ./dlls/ddraw/device.c ./dlls/dinput/tests/device.c ./dlls/dinput/device.c ./dlls/wined3d/device.c ./dlls/d3d8/tests/device.c ./dlls/d3d8/device.c ./dlls/mountmgr.sys/device.c ./dlls/d3d9/tests/device.c ./dlls/d3d9/device.c ./programs/winedevice/device.c ./server/device.c context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. 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... cleanup usually does not warrant a new function. This is exactly what we use goto for. pixelshader.c - this has a recompile goto.. again why not make this into a routine? provider.c - same This function is ugly, but not because it uses goto. The function is 144 lines long, which is too long. I'm not familiar with this code, but I'm pretty sure some factorization is in order. The level of indentation could be reduced by breaking on failure instead of proceeding on success. Assuming this previous change is made, the use of the goto would be that much more appealing. These are just the ones I have run accross so far.. Why do you say that... or is the idea.. hack and get it done... one of the problems with goto's is it makes finding and putting in patches a pain in the ass.. and thats putting it bluntly.. Complete sentences go a long way towards effective communication :-) The use of goto's cannot possibly be considered as a hack. I also don't see how you find goto's a problem when patching the code. Of course.. Do you have a suggestion on how I should word the RFC? No, just send in a patch to wine-devel stating why you think the change is correct. P.S. Make sure you reply-all to cc wine-devel. We're an open project and we communicate openly so everyone can be a part of the discussion. -- James Hawkins
Re: general question..
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: [EMAIL PROTECTED] [mailto:wine-patches- [EMAIL PROTECTED] On Behalf Of Chris Ahrendt Sent: Monday, July 21, 2008 8:42 PM To: [EMAIL PROTECTED] 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
Re: general question..
James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan
Re: general question..
Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan probably not this is the sort of discussion I was wanting to start with the GOTO comment =) Chris
Re: general question..
Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan
Re: general question..
Ivan Gyurdiev wrote: Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan Ok here is the next question.. do I just rework it into seperate functions and leave the main API routine alone and just call the others... whats the process to do something like this? Chris
Re: general question..
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?
Re: general question..
Chris Ahrendt wrote: Ivan Gyurdiev wrote: Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan Ok here is the next question.. do I just rework it into seperate functions and leave the main API routine alone and just call the others... whats the process to do something like this? Chris Unless you are trying to fix something, make it more efficient there is no reason to touch the code just because it looks bad. Some one might have lots of work done on said function and just about to send their changes in. Then come to find out that all of the changes have to be scrapped and rewritten because some one reformatted the code. And it also helps if you actually understand what that piece of code is doing. Some code is kept ugly just so one day it can be properly rewritten not just reformatted. Vitaliy.
