Re: [ros-svn] [gdalsnes] 18113: -reorder InsertXscendingOrder macro argument order and update uses
gdalsnes@svn.reactos.com wrote:
-reorder InsertXscendingOrder macro argument order and update uses -start to macrofy list enums in ntoskrnl using LIST_FOR_EACH macros -use IsListEmpty some places instead of manual coding -profile.c:KeStartProfile() zero correct buffer in (and not the NULL ptr;-) -improve LIST_FOR_EACH macros so that the iterator is set to NULL at end of enum -kthread.c:KiServiceCheck() set the shadowtable before calling the win32k proc/thread init funcs -apc.c:KiInsertQueueApc() insert special apcs in correct fifo order -apc.c:KeFlushQueueApc() simplify -timer.c:KiExpireTimers() calling RemoveEntryList on a possibly empty list is not ok
Updated files: trunk/reactos/include/reactos/helper.h trunk/reactos/ntoskrnl/io/device.c trunk/reactos/ntoskrnl/io/driver.c trunk/reactos/ntoskrnl/io/file.c trunk/reactos/ntoskrnl/io/fs.c trunk/reactos/ntoskrnl/io/irp.c trunk/reactos/ntoskrnl/io/pnpnotify.c trunk/reactos/ntoskrnl/io/pnproot.c trunk/reactos/ntoskrnl/ke/apc.c trunk/reactos/ntoskrnl/ke/bug.c trunk/reactos/ntoskrnl/ke/kqueue.c trunk/reactos/ntoskrnl/ke/kthread.c trunk/reactos/ntoskrnl/ke/profile.c trunk/reactos/ntoskrnl/ke/queue.c trunk/reactos/ntoskrnl/ke/timer.c trunk/reactos/subsys/system/usetup/partlist.c trunk/reactos/subsys/win32k/ntuser/class.c
Hi,
I'm going to have to voice my extreme outcry against this patch. It is replacing clear and easily debuggable code by non-standard macros which are impossible to debug. It is also going to create problems when people which are not used to them (such as myself):
1) Have no idea what some LIST_FOR_EACH_SAFE thing is doing vs LIST_FOR_EACH vs InsertListLifoSafeMaybeLoopOrWhoKnowsWhat 2) Will never code using these macros, and thus the source will become full of both methods, which is totally ugly.
Sometimes, especially in kernel code, it's much better to have an expanded logic then to use a macro. I wrote a great majority of the code that has just been, imho, pollutted with these macros, and I don't appreciate for someone to barge in and change my code for some macro that I don't understand, nor wish to, because it only causes problems. I take great care of the code I write and "own" it (in the programming term) so that if a bug in it arises, I can quickly identify it and be held responsible for it. The addition of these macros hinders that effort.
I don't mind someone using them inside his or her own code. I just mind their usage in ntoskrnl, and even more so when it overwrites the clean, clear code that I've written and am used to debugging.
Best regards, Alex Ionescu
Alex Ionescu wrote:
Hi,
I'm going to have to voice my extreme outcry against this patch. It is replacing clear and easily debuggable code by non-standard macros which are impossible to debug. It is also going to create problems when people which are not used to them (such as myself):
- Have no idea what some LIST_FOR_EACH_SAFE thing is doing vs
LIST_FOR_EACH vs InsertListLifoSafeMaybeLoopOrWhoKnowsWhat 2) Will never code using these macros, and thus the source will become full of both methods, which is totally ugly.
Sometimes, especially in kernel code, it's much better to have an expanded logic then to use a macro. I wrote a great majority of the code that has just been, imho, pollutted with these macros, and I don't appreciate for someone to barge in and change my code for some macro that I don't understand, nor wish to, because it only causes problems. I take great care of the code I write and "own" it (in the programming term) so that if a bug in it arises, I can quickly identify it and be held responsible for it. The addition of these macros hinders that effort.
I don't mind someone using them inside his or her own code. I just mind their usage in ntoskrnl, and even more so when it overwrites the clean, clear code that I've written and am used to debugging.
Best regards, Alex Ionescu
I definitely agree.
- Thomas
Hi,
--- Alex Ionescu ionucu@videotron.ca wrote:
I don't mind someone using them inside his or her own code. I just mind their usage in ntoskrnl, and even more so when it overwrites the clean, clear code that I've written and am used to debugging.
I think this change should be reverted as well. If Alex is maintaining this code and he does not want to use these macros I don't think we should force them on anyone. While it would be nice to use the Wine list macros in a standard way I just can't agree with it if the people that are hacking on the kernel the most have a problem with it.
Thanks Steven
__________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
If Alex doesnt like it ill revert it in the files that he "own", but this "i own the code" stuff has gone too far.
Any person can see that those macros produce much cleaner code, and without any measureable performance loss. And the stuff with "macros screw my debugging", all list manipulation functions are already macros.
Steven Edwards wrote:
Hi,
--- Alex Ionescu ionucu@videotron.ca wrote:
I don't mind someone using them inside his or her own code. I just mind their usage in ntoskrnl, and even more so when it overwrites the clean, clear code that I've written and am used to debugging.
I think this change should be reverted as well. If Alex is maintaining this code and he does not want to use these macros I don't think we should force them on anyone. While it would be nice to use the Wine list macros in a standard way I just can't agree with it if the people that are hacking on the kernel the most have a problem with it.
Thanks Steven
Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Gunnar Dalsnes wrote:
If Alex doesnt like it ill revert it in the files that he "own", but this "i own the code" stuff has gone too far.
Any person can see that those macros produce much cleaner code, and without any measureable performance loss. And the stuff with "macros screw my debugging", all list manipulation functions are already macros.
I don't think it's about "owning" code - I also strongly dislike LIST_FOR_EACH and especially LIST_FOR_EACH_SAFE. I also prefer the loops as they have been implemented before. Sometimes also such minor optimizations help a lot. And if one doesn't know how to deal with double linked lists, one shouldn't use them, IMO.
- Thomas
I also prefer the loops as they have been implemented before. Sometimes also such minor optimizations help a lot.
Walking the list by handcoding is not an optimization. Those macros are just as fast as doing it manually.
And if one doesn't know how to deal with double linked lists, one shouldn't use them, IMO.
Cleaner code (less code) is the point here, not knowing how to deal with lists. Less code = better code.
- Thomas
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Gunnar Dalsnes wrote:
Walking the list by handcoding is not an optimization. Those macros are just as fast as doing it manually.
Yes it is, not always but especially the kernel sometimes have time critical operations where any unnecessary overhead could have an impact on performance.
Cleaner code (less code) is the point here, not knowing how to deal with lists. Less code = better code.
That's not always true. I remember the old Nt/KeWaitForSingleObject implementation.... total mess and slow as hell...
At least I'm not going to use these non-standard macros.
- Thomas
Gunnar Dalsnes wrote:
If Alex doesnt like it ill revert it in the files that he "own", but this "i own the code" stuff has gone too far.
It's not about "owning". Call it "maintaining" if you want, like Steven said. And it's not "Alex" doesn't like it, it's erm, Alex, Steven, Thomas, Casper, Hartmut and probably others. I'll go in more detail on the technical reason why, at least for me, the macros are unacceptable. (Other then the fact that macros in the programming world are considered to be as bad as goto.) Here's something I can do that your macros remove:
HeadList = &Foo.Flink DPRINT1("List head at: %p\n", HeadList); Entry = HeadList->Flink; DPRINT1("Entry at: %p\n", Entry); while (Entry != HeadList) { DPRINT1("Getting object\n"); Object = CONTAINING_RECORD(Entry, OBJECT, Link); DPRINT1("Got the object: %p\n", Object);
<.... do stuff>
DPRINT1("About to remove entry from list\n"); RemoveEntryList(&Object->Link);
DPRINT1("Entry removed, moving to: %p\n", Entry->Flink); Entry = Entry->Flink; }
In the current state that ReactOS + the kernel + debugging facilities are, that is sometimes the only way to find out what's crashing during a list .
Any person can see that those macros produce much cleaner code, and without any measureable performance loss. And the stuff with "macros screw my debugging", all list manipulation functions are already macros.
No they're not. They're inlined functions. And they do insertion/removal, not parsing lists. See above why parsing should be expanded.
Best regards, Alex Ionescu
Alex Ionescu wrote:
Gunnar Dalsnes wrote:
If Alex doesnt like it ill revert it in the files that he "own", but this "i own the code" stuff has gone too far.
It's not about "owning". Call it "maintaining" if you want, like Steven said. And it's not "Alex" doesn't like it, it's erm, Alex, Steven, Thomas, Casper, Hartmut and probably others. I'll go in more detail on the technical reason why, at least for me, the macros are unacceptable. (Other then the fact that macros in the programming world are considered to be as bad as goto.) Here's something I can do that your macros remove:
HeadList = &Foo.Flink DPRINT1("List head at: %p\n", HeadList); Entry = HeadList->Flink; DPRINT1("Entry at: %p\n", Entry); while (Entry != HeadList) { DPRINT1("Getting object\n"); Object = CONTAINING_RECORD(Entry, OBJECT, Link); DPRINT1("Got the object: %p\n", Object); <.... do stuff>
DPRINT1("About to remove entry from list\n"); RemoveEntryList(&Object->Link); DPRINT1("Entry removed, moving to: %p\n", Entry->Flink); Entry = Entry->Flink;}
If you come across a crash during an enum you just temporarily change the code to hunt the bug, just like normal bug hunting. Some of the list enums i changed were on this form...
for (HeadList = &Foo.Flink; Entry != HeadList; Entry = Entry->Flink) { Object = CONTAINING_RECORD(Entry, OBJECT, Link); Object->stuff }
...and if you had to "debug" it like you showed youd had to edit the code just as well. But for normal not-in-process-of-being-debugged-code i think the above code sux (even without the dprints).
In the current state that ReactOS + the kernel + debugging facilities are, that is sometimes the only way to find out what's crashing during a list .
Any person can see that those macros produce much cleaner code, and without any measureable performance loss. And the stuff with "macros screw my debugging", all list manipulation functions are already macros.
No they're not. They're inlined functions. And they do insertion/removal, not parsing lists. See above why parsing should be expanded.
Yeah, i see now they are inlined in latest DDKs...
I'd just like to chime in here with my opinion. About a year ago I had the idea of introducing macros for error handling instead of explicit gotos. We had some discussion on the matter, and I read several papers on the web, and came to agree that use of macros that change flow control is anathma. It makes the code completely unreadable and unmaintainable.
At first it seemed like a great idea to hide the "nasty" gotos and have to type less while hammering out the code, but in the end, even when you are familliar with the macros, it looks ugly, and for someone who isn't familliar with the macros, it is utterly unreadable.
Recently I was reading over some apache code and they make extensive use of macros for flow control. I found myself becoming frustrated to no end because I could not figure out what the hell was going on. They looked like nice, neat function calls, but I could not for the life of me figure out where the code was that was being called. It wouldn't have looked so deceptively simple if they coded out what was going on explicitly instead of using macros, but it would have been easy to understand, and why should something look deceptively simple when it really isn't? That only leads to confusion when trying to understand what's going on under the hood.
Phillip Susi wrote:
I'd just like to chime in here with my opinion. About a year ago I had the idea of introducing macros for error handling instead of explicit gotos. We had some discussion on the matter, and I read several papers on the web, and came to agree that use of macros that change flow control is anathma. It makes the code completely unreadable and unmaintainable.
In win32k i use macros for flow control in the NtUser syscalls (functionally like a try/finally block) where its very important that some cleanup (release a lock) is always done:
BOOL NtFunc() { DECLARE_RETURN(BOOL);
Lock(); if (Stuff) RETURN(FALSE); .... RETURN(TRUE);
CLEANUP: Unlock(Stuff); DPRINT1("NtFunc returned %i\n", _ret_); END_CLEANUP; }
At first it seemed like a great idea to hide the "nasty" gotos and have to type less while hammering out the code, but in the end, even when you are familliar with the macros, it looks ugly, and for someone who isn't familliar with the macros, it is utterly unreadable.
If we should only invent stuff that ppl _already_ understand we just had to stop inventing.
Recently I was reading over some apache code and they make extensive use of macros for flow control. I found myself becoming frustrated to no end because I could not figure out what the hell was going on. They looked like nice, neat function calls, but I could not for the life of me figure out where the code was that was being called.
So if they had been function calls and not macros it would have been easier??? Or should we stop using functions also? ;-P
It wouldn't have looked so deceptively simple if they coded out what was going on explicitly instead of using macros, but it would have been easy to understand, and why should something look deceptively simple when it really isn't?
If you cant find a macro (simple text search) you probably wouldnt have understod the code _without_ the macros either;-P
But off course you have to learn how the macros work to understand and utilize them, but that goes for any code. I cant see how learning a function call with 10+ params (typical nt syscall func) should be any easier than learning a simple macro. Thats just being lazy.
G.
From: Gunnar Dalsnes
Phillip Susi wrote:
I'd just like to chime in here with my opinion. About a year ago I had the idea of introducing macros for error handling instead of explicit gotos. We had some discussion on the matter, and I read several papers on the web, and came to agree that use of macros that change flow control is anathma. It makes the code completely unreadable and unmaintainable.
In win32k i use macros for flow control in the NtUser syscalls (functionally like a try/finally block) where its very important that some cleanup (release a lock) is always done:
BOOL NtFunc() { DECLARE_RETURN(BOOL);
Lock(); if (Stuff) RETURN(FALSE); .... RETURN(TRUE);
CLEANUP: Unlock(Stuff); DPRINT1("NtFunc returned %i\n", _ret_); END_CLEANUP; }
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions. For you it's easy, you wrote them, but for the rest of us it is obfuscating. I didn't like it when you introduced this stuff in win32k.
Gé van Geldorp.
Ge van Geldorp wrote:
From: Gunnar Dalsnes
Phillip Susi wrote:
I'd just like to chime in here with my opinion. About a year ago I had the idea of introducing macros for error handling instead of explicit gotos. We had some discussion on the matter, and I read several papers on the web, and came to agree that use of macros that change flow control is anathma. It makes the code completely unreadable and unmaintainable.
In win32k i use macros for flow control in the NtUser syscalls (functionally like a try/finally block) where its very important that some cleanup (release a lock) is always done:
BOOL NtFunc() { DECLARE_RETURN(BOOL);
Lock(); if (Stuff) RETURN(FALSE); .... RETURN(TRUE);
CLEANUP: Unlock(Stuff); DPRINT1("NtFunc returned %i\n", _ret_); END_CLEANUP; }
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions.
Sure you do, if you try _reeeealy_ hard;-P Ok, those RETURN's set the _ret_ value and transfer control to the CLEANUP block. END_CLEANUP does the actual return with the value passed in RETURN. If you fallthru to the CLEANUP block you will assert so you must RETURN before the CLEANUP block. The CLEANUP block can be seen as an externel function but for obvious reasons it needs to be in the function... Hope this helps:-D
For you it's easy, you wrote them, but for the rest of us it is obfuscating. I didn't like it when you introduced this stuff in win32k.
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find. 2)Using gotos are much more ugly imo. 3)try/finally could be used if we supported returning from a try block, thou this is not really recomended either because of unwinding. The common solution is to goto to the end of the try block, but then we are back to start.
G.
From: Gunnar Dalsnes
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions.
Sure you do, if you try _reeeealy_ hard;-P
No, really, I don't <<without looking at the macro definitions>>. RETURN sounds much like return, it is non-obvious that they're actually goto's to CLEANUP. Ofcourse, I figured it out when you committed that stuff 3 weeks ago, but when looking at it last night it was again non-obvious to me. On the other hand, I had no problem whatsoever figuring out the macro-free code that Nathan posted:
BOOL NtFunc() { BOOL bResult; void *pPointer = NULL;
Lock();
if (Stuff) { bResult = FALSE; goto cleanup; } ....
bResult = TRUE;
cleanup: if (pPointer) free(pPointer); Unlock(stuff); DPRINT1("NtFunc returned %i\n", bResult); return bResult; }
2)Using gotos are much more ugly imo.
Oh, so goto's are acceptable if and only if you hide them out of sight?
Gé van Geldorp.
Ge van Geldorp wrote:
From: Gunnar Dalsnes
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions.
Sure you do, if you try _reeeealy_ hard;-P
No, really, I don't <<without looking at the macro definitions>>. RETURN sounds much like return, it is non-obvious that they're actually goto's to CLEANUP. Ofcourse, I figured it out when you committed that stuff 3 weeks ago, but when looking at it last night it was again non-obvious to me. On the other hand, I had no problem whatsoever figuring out the macro-free code that Nathan posted:
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block? I may very well think the the finally block is only executed if i run at the end of the try block. But i _learned_ and figured out how it works. And now i _remeber_. But its not that same you say, because the macro _can_ be implemented by hardcoding, while try/finally cannot. Uhm, try/finally in ros IS macros;-P Noone said, "kjk, s*rew you and your seh macros." "This belongs in the compiler." "I refuse to learn how to use those ugly seh macros."
BOOL NtFunc() { BOOL bResult; void *pPointer = NULL;
Lock();
if (Stuff) { bResult = FALSE; goto cleanup; } ....
bResult = TRUE;
cleanup: if (pPointer) free(pPointer); Unlock(stuff); DPRINT1("NtFunc returned %i\n", bResult); return bResult; }
2)Using gotos are much more ugly imo.
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly. I didnt make those macros so i could type less. I made they so i can _read_ less. Thats the point. Readability. When looking at code i like to quickly spot the points of return. In complex code, and if it already have gotos, its confusing. Having a reserved word like RETURN is also nice for sytax highlightning (and its actually the same _word_ as normally used for return;-). Making up a mind about what the code does very quickly is nice, and with RETURN i can do that just as fast (faster) as with return.
Im sure all of you would like those macros if you didnt refuse to learn- and use them. But as long as you do you will off course hate them. I hate all the stuff i dont understand as well.
G.
Gunnar Dalsnes wrote:
But its not that same you say, because the macro _can_ be implemented by hardcoding, while try/finally cannot. Uhm, try/finally in ros IS macros;-P Noone said, "kjk, s*rew you and your seh macros." "This belongs in the compiler." "I refuse to learn how to use those ugly seh macros."
That's because there's no other way to get SEH that we really need into ros unless either GCC feels like implementing or we switch to a decent compiler... I don't really like these macros either, especially because of their limitations. But better than nothing.
- Thomas
Gunnar Dalsnes wrote:
Ge van Geldorp wrote:
From: Gunnar Dalsnes
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions.
Sure you do, if you try _reeeealy_ hard;-P
No, really, I don't <<without looking at the macro definitions>>. RETURN sounds much like return, it is non-obvious that they're actually goto's to CLEANUP. Ofcourse, I figured it out when you committed that stuff 3 weeks ago, but when looking at it last night it was again non-obvious to me. On the other hand, I had no problem whatsoever figuring out the macro-free code that Nathan posted:
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block?
That's a compiler language feature. That's like saying that learning some 3rd party macro is equivalent to what operator new does in C++.
I may very well think the the finally block is only executed if i run at the end of the try block.
Someone that has learnt Win32 C/C++ programming and exception handling wouldn't think that.
But i _learned_ and figured out how it works. And now i _remeber_.
I also learnt and remember English. But I chose not to learn Zimbabwean.
But its not that same you say, because the macro _can_ be implemented by hardcoding, while try/finally cannot. Uhm, try/finally in ros IS macros;-P Noone said, "kjk, s*rew you and your seh macros." "This belongs in the compiler." "I refuse to learn how to use those ugly seh macros."
I think KJK has told me a million times how ugly PSEH is and how it should be in the compiler. But unlike other macros, we desperately need PSEH macros, we don't have a way around it. And their flow-control is as "hidden" as the seh intrinsics in compiler SEH.
BOOL NtFunc() { BOOL bResult; void *pPointer = NULL;
Lock();
if (Stuff) { bResult = FALSE; goto cleanup; } ....
bResult = TRUE;
cleanup: if (pPointer) free(pPointer); Unlock(stuff); DPRINT1("NtFunc returned %i\n", bResult); return bResult; }
2)Using gotos are much more ugly imo.
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly.
I think it's a great way to do
S = Foo(); if (S) { S = Foo(); if (S) { S = Foo(); if (S) { S = Foo(); } else { goto cleanup; } } else { goto cleanup; } } else { goto cleanup; }
cleanup: HeapFree(...) return Status;
instead of having the cleanup code quadriplicated.
I didnt make those macros so i could type less. I made they so i can _read_ less. Thats the point. Readability. When looking at code i like to quickly spot the points of return. In complex code, and if it already have gotos, its confusing. Having a reserved word like RETURN is also nice for sytax highlightning (and its actually the same _word_ as normally used for return;-). Making up a mind about what the code does very quickly is nice, and with RETURN i can do that just as fast (faster) as with return.
Im sure all of you would like those macros if you didnt refuse to learn- and use them. But as long as you do you will off course hate them. I hate all the stuff i dont understand as well.
That's really a flawed statement. Learning and using these macros won't change their inner deficiencies as being flow control macros. Learning and using them will just propagate a frowned-upon programming practice. Your argument is much like saying "I'm sure if you all used uninitialized variables you'll like them".
G.
Best regards, Alex Ionescu
Hi, I'm new here, if y'all don't mind I'll throw my thoughts in. I tend to do soemthing like this:
int foo(void){ int ret;
if (stuf){ ret = FALSE; goto error; }; error: /* Error clean up code */
done: return (ret); };
I think that too many marcos can clutter up code. How ever, the marcos can be useful from keeping one from repeating the same task again and again, but one has to name them so other people can tell what they do.
Alex Ionescu wrote:
Gunnar Dalsnes wrote:
Ge van Geldorp wrote:
From: Gunnar Dalsnes
And I think these macro's are a perfect example of Phillip's point. I have no idea how the flow of control is without looking at the macro definitions.
Sure you do, if you try _reeeealy_ hard;-P
No, really, I don't <<without looking at the macro definitions>>. RETURN sounds much like return, it is non-obvious that they're actually goto's to CLEANUP. Ofcourse, I figured it out when you committed that stuff 3 weeks ago, but when looking at it last night it was again non-obvious to me. On the other hand, I had no problem whatsoever figuring out the macro-free code that Nathan posted:
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block?
That's a compiler language feature. That's like saying that learning some 3rd party macro is equivalent to what operator new does in C++.
I may very well think the the finally block is only executed if i run at the end of the try block.
Someone that has learnt Win32 C/C++ programming and exception handling wouldn't think that.
But i _learned_ and figured out how it works. And now i _remeber_.
I also learnt and remember English. But I chose not to learn Zimbabwean.
But its not that same you say, because the macro _can_ be implemented by hardcoding, while try/finally cannot. Uhm, try/finally in ros IS macros;-P Noone said, "kjk, s*rew you and your seh macros." "This belongs in the compiler." "I refuse to learn how to use those ugly seh macros."
I think KJK has told me a million times how ugly PSEH is and how it should be in the compiler. But unlike other macros, we desperately need PSEH macros, we don't have a way around it. And their flow-control is as "hidden" as the seh intrinsics in compiler SEH.
BOOL NtFunc() { BOOL bResult; void *pPointer = NULL;
Lock();
if (Stuff) { bResult = FALSE; goto cleanup; } ....
bResult = TRUE;
cleanup: if (pPointer) free(pPointer); Unlock(stuff); DPRINT1("NtFunc returned %i\n", bResult); return bResult; }
2)Using gotos are much more ugly imo.
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly.
I think it's a great way to do
S = Foo(); if (S) { S = Foo(); if (S) { S = Foo(); if (S) { S = Foo(); } else { goto cleanup; } } else { goto cleanup; } } else { goto cleanup; }
cleanup: HeapFree(...) return Status;
instead of having the cleanup code quadriplicated.
I didnt make those macros so i could type less. I made they so i can _read_ less. Thats the point. Readability. When looking at code i like to quickly spot the points of return. In complex code, and if it already have gotos, its confusing. Having a reserved word like RETURN is also nice for sytax highlightning (and its actually the same _word_ as normally used for return;-). Making up a mind about what the code does very quickly is nice, and with RETURN i can do that just as fast (faster) as with return.
Im sure all of you would like those macros if you didnt refuse to learn- and use them. But as long as you do you will off course hate them. I hate all the stuff i dont understand as well.
That's really a flawed statement. Learning and using these macros won't change their inner deficiencies as being flow control macros. Learning and using them will just propagate a frowned-upon programming practice. Your argument is much like saying "I'm sure if you all used uninitialized variables you'll like them".
G.
Best regards, Alex Ionescu _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block?
That's a compiler language feature. That's like saying that learning some 3rd party macro is equivalent to what operator new does in C++.
The new operator is just a word as anything else. Just because its a compiler feature doesnt make it "magic". It just means that every C++ compiler should reserve and support it just like we can say ReactOS reserve and support Xxx for usage Xxx. And you can overload new u know. Then you never know what it _really_ does;-P
I also learnt and remember English. But I chose not to learn Zimbabwean.
It this context it would be "Zimbabwean sound so weird I refuse to learn it. Zimbabwean is flawed and ppl should stop speaking it. They should learn English instead so I can understand them."
instead of having the cleanup code quadriplicated.
At least we agree on something.
So then Im free to apply that schema thruout ros? Or will I then get: "gotos sux", "please dont do this", "it looks so ugly", "i refuse to do it this way" etc?
That's really a flawed statement. Learning and using these macros won't change their inner deficiencies as being flow control macros.
They are just as deficient as the goto example you showed ei. equivalent.
Learning and using them will just propagate a frowned-upon programming practice.
Just because someone else says you should do so doesnt make it right. Thinking for urself (open mind) and not caring about what others says ("the standard") can be a relief.
Your argument is much like saying "I'm sure if you all used uninitialized variables you'll like them".
It depends. In Java they would;-P
G.
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work. Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either? At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
Richard
Gunnar Dalsnes wrote:
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block?
That's a compiler language feature. That's like saying that learning some 3rd party macro is equivalent to what operator new does in C++.
The new operator is just a word as anything else. Just because its a compiler feature doesnt make it "magic". It just means that every C++ compiler should reserve and support it just like we can say ReactOS reserve and support Xxx for usage Xxx. And you can overload new u know. Then you never know what it _really_ does;-P
I also learnt and remember English. But I chose not to learn Zimbabwean.
It this context it would be "Zimbabwean sound so weird I refuse to learn it. Zimbabwean is flawed and ppl should stop speaking it. They should learn English instead so I can understand them."
instead of having the cleanup code quadriplicated.
At least we agree on something.
So then Im free to apply that schema thruout ros? Or will I then get: "gotos sux", "please dont do this", "it looks so ugly", "i refuse to do it this way" etc?
That's really a flawed statement. Learning and using these macros won't change their inner deficiencies as being flow control macros.
They are just as deficient as the goto example you showed ei. equivalent.
Learning and using them will just propagate a frowned-upon programming practice.
Just because someone else says you should do so doesnt make it right. Thinking for urself (open mind) and not caring about what others says ("the standard") can be a relief.
Your argument is much like saying "I'm sure if you all used uninitialized variables you'll like them".
It depends. In Java they would;-P
G. _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Richard Campbell wrote:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work.
Having common cleanup is _very_ good programming practice, and in lack of working/efficient try/finally i mean that using gotos and/or macros for this highy outweights the (alleged) bad practice of using gotos and/or macros.
Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either?
Have a look at any medium to large function in ros and youll pretty fast find points of return (in case of error/fail) where it fails to do some cleanup. Having a common place for cleanup makes it easier to make this right.
Example: kernel32\process\create.c:BasepInitializeEnvironment(). return at line 496 & 574: fail to destroy proces params
At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
Thats exactly what macros _should_ be used for. Hiding mess. Im sure macros, gotos etc. can be abused but imo this is not the case here.
G.
And there are methods other then using goto that allow for common cleanup. One such example is:
int somefunction() { BOOL bSuccess;
if ( someotherfunction(arg) ) { if ( yetanotherfunction(arg) ) { bSuccess = TRUE; } } CleanUpCode; }
Anyways, back to the point. Most devs have already argued against the idea of using macros. I say the discussion stops and we call a vote.
Gunnar Dalsnes wrote:
Richard Campbell wrote:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work.
Having common cleanup is _very_ good programming practice, and in lack of working/efficient try/finally i mean that using gotos and/or macros for this highy outweights the (alleged) bad practice of using gotos and/or macros.
Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either?
Have a look at any medium to large function in ros and youll pretty fast find points of return (in case of error/fail) where it fails to do some cleanup. Having a common place for cleanup makes it easier to make this right.
Example: kernel32\process\create.c:BasepInitializeEnvironment(). return at line 496 & 574: fail to destroy proces params
At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
Thats exactly what macros _should_ be used for. Hiding mess. Im sure macros, gotos etc. can be abused but imo this is not the case here.
G. _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
I say Majority Rules, and the code is reverted. But that's me.
On 9/28/05, Richard Campbell eek2121@comcast.net wrote:
And there are methods other then using goto that allow for common cleanup. One such example is:
int somefunction() { BOOL bSuccess;
if ( someotherfunction(arg) ) { if ( yetanotherfunction(arg) ) { bSuccess = TRUE; } } CleanUpCode;}
Anyways, back to the point. Most devs have already argued against the idea of using macros. I say the discussion stops and we call a vote.
Gunnar Dalsnes wrote:
Richard Campbell wrote:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work.
Having common cleanup is _very_ good programming practice, and in lack of working/efficient try/finally i mean that using gotos and/or macros for this highy outweights the (alleged) bad practice of using gotos and/or macros.
Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either?
Have a look at any medium to large function in ros and youll pretty fast find points of return (in case of error/fail) where it fails to do some cleanup. Having a common place for cleanup makes it easier to make this right.
Example: kernel32\process\create.c:BasepInitializeEnvironment(). return at line 496 & 574: fail to destroy proces params
At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
Thats exactly what macros _should_ be used for. Hiding mess. Im sure macros, gotos etc. can be abused but imo this is not the case here.
G. _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
-- "I had a handle on life, but then it broke"
TwoTailedFox wrote:
I say Majority Rules, and the code is reverted. But that's me.
I'm going to revert the macros in the code I maintain sometime this week... I've been working on getting Winsock 2.2 running so I haven't had much time.
Best regards, Alex Ionescu
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of Alex Ionescu Sent: 29. september 2005 00:13 To: TwoTailedFox; ReactOS Development List Subject: Re: [ros-dev] Re: [ros-svn] [gdalsnes]18113:-reorderInsertXscendingOrder macro argument order and update uses
TwoTailedFox wrote:
I say Majority Rules, and the code is reverted. But that's me.
I'm going to revert the macros in the code I maintain sometime this week... I've been working on getting Winsock 2.2 running so I haven't had much time.
Best regards, Alex Ionescu
No, that would leed to even more inconsistencies in ReactOS. We should vote on the issue and take appropriate action depending on the result of the voting.
Casper
Casper Hornstrup wrote:
No, that would leed to even more inconsistencies in ReactOS. We should vote on the issue and take appropriate action depending on the result of the voting.
Casper
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff. You can't go ahead, piss on someone's code, and then have a vote if the person that got pissed on should get his code back or not. What kind of screwed up system is that? Go count the threads itself, I've counted 15 people who agreed that:
1) Macros are bad, especially in ntoskrnl 2) Overwriting someone's maintained code is bad.
Regardless of the outcome of this "vote", the 4 files that I wrote 99.99% of will be returned to their original state by me, it's my right.
Best regards, Alex Ionescu
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of Alex Ionescu Sent: 29. september 2005 19:02 To: ReactOS Development List Subject: Re: [ros-dev] Re: [ros-svn][gdalsnes]18113:-reorderInsertXscendingOrder macro argument orderand update uses
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff. You can't go ahead, piss on someone's code, and then have a vote if the person that got pissed on should get his code back or not. What kind of screwed up system is that? Go count the threads itself, I've counted 15 people who agreed that:
- Macros are bad, especially in ntoskrnl
- Overwriting someone's maintained code is bad.
Voting is the best decision making procedure we've found so far. Overwriting someone else's code just because you don't like it is counter productive. If there are arguments for not implementing something in a particular way then provide those arguments, vote on the subject, and document the result in wiki. Whenever you remove code that violates a rule in the future, you only need to reference the relevant section in wiki to have the "right" to remove the code. There is no need to fight over which code is "best" or which code is more "right". The rule is in wiki and it applies until replaced by a new rule. If there is no formal rule, then there is nothing stopping another developer from replacing your replaced code. This could go on forever or until all developers, but one give up, wasting a lot of the (already limited) resources of the project.
Regardless of the outcome of this "vote", the 4 files that I wrote 99.99% of will be returned to their original state by me, it's my right.
ReactOS has collective code ownership. This implies that all developers own all code and you have no more right to change a piece of code than anyone else which is taking part of the project. If you want to see module ownership instead then propose a solution for this and call for a vote on the subject.
Casper
Alex Ionescu wrote:
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff.
"Revert wars: not just for Wikipedia anymore"
Revert Wars, ironically, end after a vote on Wikipedia ;)
On 9/29/05, KJKHyperion hackbunny@reactos.com wrote:
Alex Ionescu wrote:
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff.
"Revert wars: not just for Wikipedia anymore" _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
-- "I had a handle on life, but then it broke"
I have a great question or solution to the problem:
Why not compare the two versions of the code. Test for speed and stability and then use which ever one is stable and fast. The more someone bickers about a particular subject the more won't get done. There are lots of different things people can be doing besides senseless arguing on the mailing list.
I have been working on implementing system services for a while now, kind of stuck but it should pick up here in a little bit. On Sep 29, 2005, at 2:59 PM, TwoTailedFox wrote:
Revert Wars, ironically, end after a vote on Wikipedia ;)
On 9/29/05, KJKHyperion hackbunny@reactos.com wrote:
Alex Ionescu wrote:
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff.
"Revert wars: not just for Wikipedia anymore" _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
-- "I had a handle on life, but then it broke"
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Rick Langschultz rlangschultz@cox.net (Home) rlangschultz@ellemaespa.com (Work) rlangschultz@email.uophx.edu (School)
Go ahead and have your vote, but I'm simply going to overwrite their usage in my code when I commit new stuff. You can't go ahead, piss on someone's code
Addition of the macros was done as an improvement, not to piss on (your) code. I just started at a random place in ntoskrnl (io) and took most files there.
G.
My question is: Why repeat the error exit code again and again? If used wisely, using goto statements can reduce the in a function by a few bytes and reduce the number of points of change. Intead of having to look for all the error code scattered in a function, the error code is in one spot where you don't have to change sevel lines of code scadered throughout a fuction just to change the exiting behavior. "Crashfourit" On 9/28/05, Richard Campbell eek2121@comcast.net wrote:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work. Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either? At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
Richard
Gunnar Dalsnes wrote:
Yes, but how is this different from someone not knowing/understanding that a finally block is called when returning from a try block?
That's a compiler language feature. That's like saying that learning some 3rd party macro is equivalent to what operator new does in C++.
The new operator is just a word as anything else. Just because its a compiler feature doesnt make it "magic". It just means that every C++ compiler should reserve and support it just like we can say ReactOS reserve and support Xxx for usage Xxx. And you can overload new u know. Then you never know what it _really_ does;-P
I also learnt and remember English. But I chose not to learn
Zimbabwean.
It this context it would be "Zimbabwean sound so weird I refuse to learn it. Zimbabwean is flawed and ppl should stop speaking it. They should learn English instead so I can understand them."
instead of having the cleanup code quadriplicated.
At least we agree on something.
So then Im free to apply that schema thruout ros? Or will I then get: "gotos sux", "please dont do this", "it looks so ugly", "i refuse to do it this way" etc?
That's really a flawed statement. Learning and using these macros won't change their inner deficiencies as being flow control macros.
They are just as deficient as the goto example you showed ei.
equivalent.
Learning and using them will just propagate a frowned-upon programming practice.
Just because someone else says you should do so doesnt make it right. Thinking for urself (open mind) and not caring about what others says ("the standard") can be a relief.
Your argument is much like saying "I'm sure if you all used uninitialized variables you'll like them".
It depends. In Java they would;-P
G. _______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
-- <P>My DeviantArt.com page: <A href="http://crashfourit.deviantart.com/" > http://crashfourit.deviantart.com/</A><BR>My FanFiction.net bio page: <A href="http://www.fanfiction.net/u/726606/" > http://www.fanfiction.net/u/726606/</A><BR>My Blog: <A href="http://crashfourit.blogspot.com"
http://crashfourit.blogspot.com</A><BR>America's Debate: <A
href="http://www.americasdebate.com/"
http://www.americasdebate.com/</A> </P>
Richard Campbell writes:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work. Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either? At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
While the previous examples can ge accomplished cleanly without gotos and/or macros, the rules can be different when the code is far more complicated. For instance, the handling of error conditions buried deep within multiple for loops and other statements. Take the following example:
BOOL NtFunc() { BOOL bSuccess = FALSE; LPVOID *pPointer = NULL; LPVOID *pAnotherPointer = NULL;
for (...) { if (ThisOperationCanFail()) goto cleanup;
pPointer = malloc(...); if (!pPointer) goto cleanup;
while (...) { if (ThisOperationCanFailAlso()) goto cleanup; .... if (!pAnotherPointer) { pAnotherPointer = malloc(...); if (!pPointer) goto cleanup;
if (YetAnotherCanFail()) goto cleanup; ... } } free(pPointer); pPointer = NULL; } bSuccess = TRUE; cleanup: if (pPointer) free(pPointer); if (pAnotherPointer) free(pAnotherPointer); return bSuccess; }
pPointer and pAnotherPointer are both pointers with different lifetimes; making things a bit messy. While it is possible to write such code without gotos or macros, it would involve checking and freeing those pointers from many different places. Using a cleanup label provides a convenient place to centrally locate all cleanup code.
-N
Nathan Woods wrote:
Richard Campbell writes:
Hate to throw my 2 cents in, but why use macros or goto statements at all? None of the demonstrated code actually needs a goto statement to work. Granted i've not seen the actual offensive code , but all examples here can be written without goto statements or macros. Why bother using either? At any rate, i'm inclined to agree that macros are a bad idea. Hiding a mess behind a preprocessor is considered bad coding practice.
While the previous examples can ge accomplished cleanly without gotos and/or macros, the rules can be different when the code is far more complicated. For instance, the handling of error conditions buried deep within multiple for loops and other statements. Take the following example:
BOOL NtFunc() { BOOL bSuccess = FALSE; LPVOID *pPointer = NULL; LPVOID *pAnotherPointer = NULL;
for (...) { if (ThisOperationCanFail()) goto cleanup;
pPointer = malloc(...); if (!pPointer) goto cleanup; while (...) { if (ThisOperationCanFailAlso()) goto cleanup; .... if (!pAnotherPointer) { pAnotherPointer = malloc(...); if (!pPointer) goto cleanup; if (YetAnotherCanFail()) goto cleanup; ... } } free(pPointer); pPointer = NULL;} bSuccess = TRUE; cleanup: if (pPointer) free(pPointer); if (pAnotherPointer) free(pAnotherPointer); return bSuccess; }
pPointer and pAnotherPointer are both pointers with different lifetimes; making things a bit messy. While it is possible to write such code without gotos or macros, it would involve checking and freeing those pointers from many different places. Using a cleanup label provides a convenient place to centrally locate all cleanup code.
Of course the real solution to this problem is the following simple C++ class (which I'm not really arguing we should use... I'm just throwing some gasoline on the flames :-)
template<class T> class NPagedPointer { public: inline NPagedPointer(T* p) : m_ptr(p) {} inline ~NPagedPointer() { if ( m_ptr ) ::ExFreePool(m_ptr); }
inline operator T*() { return m_ptr; } };
BOOL NtFunc() { NPagedPointer<PLUGH> pAnotherPointer;
for (...) { if (ThisOperationCanFail()) return FALSE;
NPagedPointer<XYZZY> pPointer = malloc(...); if ( ! pPointer ) return FALSE;
while (...) { if (ThisOperationCanFailAlso()) return FALSE;
....
if ( ! pAnotherPointer) { pAnotherPointer = malloc(...); if ( pAnotherPointer ) return FALSE; if (YetAnotherCanFail()) return FALSE; ... } } }
return TRUE; }
See, no cleanup code needed :-)
Of course, people don't like to use C++ in the kernel, and not everyone has agreed to learn C++ (anymore than they've agreed to learn Zimbobwean.) And it can be tricky to make sure the C++ compiler doesn't mess you up (like putting the generated template code in a paged section if you have one.)
Thanks,
Joseph
Gunnar Dalsnes wrote:
The new operator is just a word as anything else. Just because its a compiler feature doesnt make it "magic". It just means that every C++ compiler should reserve and support it just like we can say ReactOS reserve and support Xxx for usage Xxx. And you can overload new u know. Then you never know what it _really_ does;-P
I think you missed the point. The fact of the matter is that new is a part of standard C++. Every programmer who knows C++ can understand your code when you stick to standard C++. When you start obfuscating the code with macros, it becomes much harder to understand, and in effect, you aren't really speaking C++ anymore because anyone fluent in C++ can't read your code. It's like if you were to overload operator+ and have it do something completely unrelated to adding two things together. Sure, you can do it, and sure, if you understand the implementation of that operator+ you can figure out what the code is doing, but it is in no way obvious to someone who knows C++ and has the expectation that an operator+ adds. Hard to read code is bad code.
It this context it would be "Zimbabwean sound so weird I refuse to learn it. Zimbabwean is flawed and ppl should stop speaking it. They should learn English instead so I can understand them."
Again you miss the point entirely. I don't learn Zimbabwean because everyone I need to speak with understands English perfectly fine. *YOU* speak English perfectly fine. Just because you want to speak Zimbabwean does not mean that I should learn it. Sure, I could, but why should I waste the time learning it when there is no gain? You and I already can communicate in English, switching to Zimbabwean has no real benefit, so me learning it is only a waste of time. You trying to FORCE me to learn it is well, insulting.
So then Im free to apply that schema thruout ros? Or will I then get: "gotos sux", "please dont do this", "it looks so ugly", "i refuse to do it this way" etc?
Lots of code in ReactOS uses gotos in the way I described before. As all beginning programming students are taught, gotos are evil, but sometimes they are the lesser evil. This type of error handling in C is a time when CAREFUL use of gotos is less evil than the alternative. It avoids duplicating cleanup code while remaining clear and concise, which is why it is an industry standard practice. If you search you can find plenty of code out there that uses it, and plenty of best practices material that advises it.
They are just as deficient as the goto example you showed ei. equivalent.
No, they aren't. The goto is defficient because it breaks the control of flow. Exactly how it breaks it though, is obvious to anyone who speaks the language. The alternative to breaking flow control is to duplicate cleanup code, and that is a greater evil bacuse it produces even harder to maintain code than using the gotos. The macros not only break flow control, but they do so in a non obvious way, which makes them more evil than the gotos. That increase in evil is not balenced by an increase in good, so it is a bad decision.
Just because someone else says you should do so doesnt make it right. Thinking for urself (open mind) and not caring about what others says ("the standard") can be a relief.
You seem to be missing the entire point of this conversation. You are not a unique snowflake. Being different for the sake of being different is not a good thing. Ignoring others when you are trying to work with them is not a good thing. You are not the first person to have this idea. A great many before you have had this idea, and finally come to realize why it is bad. That is the reason that use of gotos and not flow control macros have become industry standard. While one should not blindly follow standards without question, there are very good reasons the standards exist, so unless you have a better reason not to follow it, you should.
In this case, following the generally accepted standards produces more readable code, and as a programmer, that should be one of your priorities. Sometimes breaking with convention can be a good thing, but such a decision needs to be weighed carefully and taken only if the benefits outweigh the losses. In this case, the only benefit to use of flow control macros is they save you a few keystrokes when writing the code, and you find it aesthetically pleasing. Of course you do, you wrote it.
With no real benefit the needs of the many outweigh your aesthetic desire to use the macros. The readability of the code to others and its maintainability in the future are far more important than your aesthetics.
Have a look at any medium to large function in ros and youll pretty fast find points of return (in case of error/fail) where it fails to do some cleanup. Having a common place for cleanup makes it easier to make this right.
Yes, having a common return point is a good programming practice, but again, when choosing HOW to have a common point of return, the method that is clearest and simplest and best understood produces the best code.
Thats exactly what macros _should_ be used for. Hiding mess. Im sure macros, gotos etc. can be abused but imo this is not the case here.
That IS the case here. Macros should _ONLY_ be used to hide a mess when doing do has some pretty strong benefits to outweigh the complexity it adds. In this case, the macro does not have any real benefit, therefore the added complexity with no benefit constitutes abuse.
Phillip Susi wrote:
Gunnar Dalsnes wrote:
The new operator is just a word as anything else. Just because its a compiler feature doesnt make it "magic". It just means that every C++ compiler should reserve and support it just like we can say ReactOS reserve and support Xxx for usage Xxx. And you can overload new u know. Then you never know what it _really_ does;-P
I think you missed the point.
You take this too serious. Didnt you see the smiley?
<snip>
Lots of code in ReactOS uses gotos in the way I described before. As all beginning programming students are taught, gotos are evil, but sometimes they are the lesser evil. This type of error handling in C is a time when CAREFUL use of gotos is less evil than the alternative. It avoids duplicating cleanup code while remaining clear and concise, which is why it is an industry standard practice. If you search you can find plenty of code out there that uses it, and plenty of best practices material that advises it.
SOME functions does, but i can probably count them on one hand (you wrote those?). To beeing an industri standard as you claim it surely hasnt cought on.
<snip>
First, Im not forcing the RETURN macros on anyone (I am trying to force the list macros, but its for your own good;-P) I applied those RETURN macros in win32k as a part of a locking rewrite and until i myself presented them as an alternative to gotos, I havent heard a damn thing about it.
Again, here are the benefits with RETURN: 1) easy to use. looks like regular return but different enough to make people understand they are different;-P 2) much easier to read and spot exit points compared to gotos that jump to various return labels (done, error, cleanup, exit, cleanup2, cleanup3 etc) mixed with "regular" gotos. 3) syntax highlight RETURN and you see a whole new world. 4) a proposed standard way of doing cleanup. not dozens of different goto labels (done, error, cleanup, exit, cleanup2, cleanup3 etc).
G.
From: Gunnar Dalsnes
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly.
The definition of your RETURN():
#define RETURN(value) { _ret_ = value; goto _cleanup_; }
which seems to set a retval and then does a goto to the end. So, again, "ugly code" (your words, not mine) is acceptable if you hide it out of sight? BTW, the following code which looks perfectly acceptable won't compile correctly with your definition:
if (somecondition) RETURN(errorcode); else do_something_else();
I admit this is a bit contrived example, you don't need the "else".
Gé van Geldorp.
Ge van Geldorp wrote:
From: Gunnar Dalsnes
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly.
The definition of your RETURN():
#define RETURN(value) { _ret_ = value; goto _cleanup_; }
which seems to set a retval and then does a goto to the end. So, again, "ugly code" (your words, not mine) is acceptable if you hide it out of sight?
Yes. Hiding it _is_ the point. Readability is what its all about. Not the code behind the names. I dont care whats behind a name/operator (compiler builtin, assembly, function call, macro etc.) as long as it do what i want and have a (in this case) _uniqueue_ name that describes what it does. Imo, "RETURN(45)" look better than "ret = 45; goto cleanup;".
BTW: As for thinking about what low level code lies behind a name i remeber the old(?) "i dont want to use while(TRUE) because it produce shitty asm, so i use for(;;) instead". I don't know if anyone thinks that anymore, but those who did needs a good spanking;-P
BTW, the following code which looks perfectly acceptable won't compile correctly with your definition:
if (somecondition) RETURN(errorcode); else do_something_else();
I admit this is a bit contrived example, you don't need the "else".
Finally some constructive critisism;-P
It can be solved like this:
#define RETURN(value) do {_ret_ = value; goto _cleanup_; }while(0)
Gé van Geldorp.
G.
Gunnar Dalsnes wrote:
Ge van Geldorp wrote:
From: Gunnar Dalsnes
Oh, so goto's are acceptable if and only if you hide them out of sight?
No, i think gotos are ok internally but i dont like them for return. First set a retval and then goto to the end. ugh...ly.
The definition of your RETURN():
#define RETURN(value) { _ret_ = value; goto _cleanup_; }
which seems to set a retval and then does a goto to the end. So, again, "ugly code" (your words, not mine) is acceptable if you hide it out of sight?
Yes. Hiding it _is_ the point.
You've just proven yourself what's wrong with flow control macros. Do you really think that in the sorry state in which Win32K is, it really needs macros to HIDE what's going on? When we can't even show a font properly with -O2? When we still have random graphics corruption? When we.. (I could go on for ages).
Readability is what its all about.
Readability != Hiding code.
Not the code behind the names. I dont care whats behind a name/operator (compiler builtin, assembly, function call, macro etc.) as long as it do what i want and have a (in this case) _uniqueue_ name that describes what it does. Imo, "RETURN(45)" look better than "ret = 45; goto cleanup;".
"I"mo. Not the "o" of 15 other people. So don't force it.
BTW: As for thinking about what low level code lies behind a name i remeber the old(?) "i dont want to use while(TRUE) because it produce shitty asm, so i use for(;;) instead". I don't know if anyone thinks that anymore, but those who did needs a good spanking;-P
BTW, the following code which looks perfectly acceptable won't compile correctly with your definition:
if (somecondition) RETURN(errorcode); else do_something_else();
I admit this is a bit contrived example, you don't need the "else".
Finally some constructive critisism;-P
It can be solved like this:
#define RETURN(value) do {_ret_ = value; goto _cleanup_; }while(0)
Gé van Geldorp.
G.
Best regards, Alex Ionescu
Yes. Hiding it _is_ the point.
You've just proven yourself what's wrong with flow control macros. Do you really think that in the sorry state in which Win32K is, it really needs macros to HIDE what's going on? When we can't even show a font properly with -O2? When we still have random graphics corruption? When we.. (I could go on for ages).
I can't see how the poor state of win32k has anything to do with it. Returning from a function isnt exactly rocket science, and the amount of code and logic hidden is minimal.
Readability is what its all about.
Readability != Hiding code.
readability: writing (print or handwriting) that can be easily read.
Im pretty sure its easier to read something if its less to read. By your definition, asm would give total readability;-P
Not directly related, but moving some repeated code into a function is "hiding" code, and it makes the code more readable, reuseable and maintainable.
G.
Gunnar Dalsnes wrote:
You've just proven yourself what's wrong with flow control macros. Do you really think that in the sorry state in which Win32K is, it really needs macros to HIDE what's going on? When we can't even show a font properly with -O2? When we still have random graphics corruption? When we.. (I could go on for ages).
I can't see how the poor state of win32k has anything to do with it. Returning from a function isnt exactly rocket science, and the amount of code and logic hidden is minimal.
I'm with Gunnar on this. It's a pattern (system calls in general are pattern-heavy), it's better to codify it. Microsoft would approve
This is your bad assumption. I can write a sentence that is 12 words and 100 characters long, or I can write it using only 6 words that are 50 characters long. If those 6 words aren't in common usage, then most people won't understand it when they see it, and so it is harder to read even though it is shorter.
To show another example in C, I can write:
if( condition1 ) { dosomething1(); if( condition2 ) if( condition 3 ) dosomething3(); else dosomething2(); }
or I can write code that does the same thing, but is shorter:
condition1 && dosomething1(), condition3 ? dosomething3 : dosomething2();
Yes, the latter code is shorter, but the former is far easier to read and maintain.
To try and steer this back to the original issue, the LIST_FOR_EACH macro causes nonintuitive branching which is hard to understand when you read it, and when working on the code, as Alex said, you often have to demacrofy it which can introduce more errors. These are legitimate down sides to using the macro, so again, you need a good reason to use it.
So far you seem to only keep insisting that the macros look better to you, so I say again, pleasing your sense of aesthetics does not outweigh the loss of maintainability.
Gunnar Dalsnes wrote:
readability: writing (print or handwriting) that can be easily read.
Im pretty sure its easier to read something if its less to read. By your definition, asm would give total readability;-P
Phillip Susi wrote:
To try and steer this back to the original issue, the LIST_FOR_EACH macro causes nonintuitive branching which is hard to understand when you read it
Lerning it is well worth the effort.
and when working on the code, as Alex said, you often have to demacrofy it which can introduce more errors.
This is the only valid point and to my defence i have said that some of the loops i replaced were for loops and here he would have exactly the same problem.
These are legitimate down sides to using the macro, so again, you need a good reason to use it.
So far you seem to only keep insisting that the macros look better to you, so I say again, pleasing your sense of aesthetics does not outweigh the loss of maintainability.
As said before, there really isnt a way to maintain walking a list. This is code printed in stone.
G.
The list macros are widely used in both Wine (4 different macros) and in the linux kernel http://www.gelato.unsw.edu.au/~dsw/public-files/kernel-docs/kernel-api/r802.... (i counted 16 different list walking macros).
A googled for a while looking for similar discussions about using such macros or not, but found none...
Seems like people who hate macros are the people who are drawn to reactos. I dont know what this means but it surely is weird. I hope it doesnt mean that reactos coders are too stupid to learn macros;-P
Well, at least i tried. Better luck next time (/me pat self on shoulder)
G.
Gunnar Dalsnes wrote:
The list macros are widely used in both Wine (4 different macros) and in the linux kernel http://www.gelato.unsw.edu.au/~dsw/public-files/kernel-docs/kernel-api/r802.... (i counted 16 different list walking macros).
I hope the Linux Kernel won't ever become an example of how an NT micro-kernel shoudl be written..
A googled for a while looking for similar discussions about using such macros or not, but found none...
Really? Let me point you to some of the famous threads by our Microsoft friends, including some of their most distinguished engineers:
http://blogs.msdn.com/larryosterman/archive/2004/12/01/273160.aspx
"
Larry: You said Macros work to hide the complexity and say so like it is a bad thing.. ? Excuse me but I thought that was the POINT of using a Macro..
Actually, in the world in which I live (writing systems programs that exist in the working set of hundreds of millions of users), hiding complexity is a very, very bad thing.
You need to be VERY careful whenever you do something that hides complexity, because it's likely to come back and bite you on the behind."
Also: "Rules of software engineering" touches on this: http://blogs.msdn.com/larryosterman/archive/2004/04/06/108686.aspx
Seems like people who hate macros are the people who are drawn to reactos.
Seems like people who hate macros are the people who are drawn to NT, too. I'm glad 99.9% of the developers here are on that line of though.
I dont know what this means but it surely is weird. I hope it doesnt mean that reactos coders are too stupid to learn macros;-P
Yeah, go tell Larry Osterman he's too stupid to learn a macro.
Well, at least i tried. Better luck next time (/me pat self on shoulder)
Read Larry's posts. Read them again. Read the comments by other MS engineers. If you still think hiding complexity is a good thing, God help you.
G.
Best regards, Alex Ionescu
Hiding complexity is a good thing. It allows you to see the big picture. (As long as its modularized).
I think you need to specify "hiding complexity in ...something... ", to make this statement valid.
I don't see why you can't have a cleanup function, aka something like CleanUp3Pointers(Ptr1,Ptr2,Ptr3).
DISCLAIMER: I'm in college, and might not know what I am talking about.
Alex Ionescu wrote:
Gunnar Dalsnes wrote:
The list macros are widely used in both Wine (4 different macros) and in the linux kernel http://www.gelato.unsw.edu.au/~dsw/public-files/kernel-docs/kernel-api/r802.... (i counted 16 different list walking macros).
I hope the Linux Kernel won't ever become an example of how an NT micro-kernel shoudl be written..
Using macros or not really has nothing to do with the kernel design... Its about the people who code.
A googled for a while looking for similar discussions about using such macros or not, but found none...
Really? Let me point you to some of the famous threads by our Microsoft friends, including some of their most distinguished engineers:
http://blogs.msdn.com/larryosterman/archive/2004/12/01/273160.aspx
"
Larry: You said Macros work to hide the complexity and say so likeit is a bad thing.. ? Excuse me but I thought that was the POINT of using a Macro..
Actually, in the world in which I live (writing systems programs that exist in the working set of hundreds of millions of users), hiding complexity is a very, very bad thing.
You need to be VERY careful whenever you do something that hides complexity, because it's likely to come back and bite you on the behind."
Also: "Rules of software engineering" touches on this: http://blogs.msdn.com/larryosterman/archive/2004/04/06/108686.aspx
This is not directly related to using list macros. I hate generic banning: "gotos considered harmful". bah... Also, for being careful, id say that the fact that Linux and Wine have used list macros extensively for ages and it works well, thats a pretty good pointer.
Seems like people who hate macros are the people who are drawn to reactos.
Seems like people who hate macros are the people who are drawn to NT, too. I'm glad 99.9% of the developers here are on that line of though.
I dont know what this means but it surely is weird. I hope it doesnt mean that reactos coders are too stupid to learn macros;-P
Yeah, go tell Larry Osterman he's too stupid to learn a macro.
The problem with MS and macros i can guess is: for foss projects, people who code are devoted to what they do and they only do it for fun (and recognizion). For a big corp that pays people to code, the devotion is probably not that high (not the fun either) and the flow of programmers is _much_ higher (not necesarely in and out of ms, but between different projects). Thats why i understand Larry. He want to protect the source agains incompetent programmers. I guess you could say that we have the same problem, we could get new incompetent members? Well, we can kick them hard and long, and also we monitor commits and we review the code.
Well, at least i tried. Better luck next time (/me pat self on shoulder)
Read Larry's posts. Read them again. Read the comments by other MS engineers. If you still think hiding complexity is a good thing, God help you.
Just because we are cloning thei OS doesnt mean we should clone their opinions, meanings and thoughts. There might still be hope for you thou;-P
G.
G.
Best regards, Alex Ionescu
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Really? Let me point you to some of the famous threads by our Microsoft friends, including some of their most distinguished engineers:
http://blogs.msdn.com/larryosterman/archive/2004/12/01/273160.aspx
I read it, and there are just as many posts against his as with him.
Larrys "point" is that "one line of code (in this case a macro) generated 1kb of code and it made the binariy huge". Also, larry thinks using vector is just as bad, so you shouldnt code in standard C++, ever! C++ is banned also!
"/me: But Larry, i have a small macro and i _know_ it wont generate more code compared to manual writing. Its a real simple macro. Larry: Macros are evil i say! Never, ever use them! Never! /me: but Windows exes are alredy huge and Windows use lots of memory. It doesnt look like not using macros have helped for Windows? Larry: Uhm..."
A little puppet show for u there;-P
G.
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
Casper
Using smaller functions instead when we are only using the code once, may help on readability but can cost speed and space. (Think of what standard code C uses to enter and exit a function.) But then again, you can get around that by using declaring those functions as inline.
On 9/28/05, Casper Hornstrup ch@csh-consult.dk wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
Casper
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
-- <P>My DeviantArt.com page: <A href="http://crashfourit.deviantart.com/"> http://crashfourit.deviantart.com/</A><BR>My FanFiction.net bio page: <A href="http://www.fanfiction.net/u/726606/"> http://www.fanfiction.net/u/726606/</A><BR>My Blog: <A href=" http://crashfourit.blogspot.com">http://crashfourit.blogspot.com</A><BR>America's Debate: <A href="http://www.americasdebate.com/"> http://www.americasdebate.com/</A> </P>
Oh please don't go there. Do you honestly think you (in general) are smarter at
generating microprocessor instructions than a multi-million hour compiler software?
If the execution speed is acceptable, I will always prefer readability of the source code
over execution speed. I don't really care if some function can be executed 1ns faster if
several functions were collapsed into one.
If I could trade a 1 second added boot up time (~8 second total then?) for readable
ReactOS source code, I would do it immediately ;-)
Casper
_____
From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of crashofurit Sent: 28. september 2005 17:22 To: ReactOS Development List Subject: Re: [ros-dev] Re: [ros-svn] [gdalsnes]18113:-reorderInsertXscendingOrder macro argument order andupdate uses
Using smaller functions instead when we are only using the code once, may help on readability but can cost speed and space. (Think of what standard code C uses to enter and exit a function.) But then again, you can get around that by using declaring those functions as inline.
On 9/28/05, Casper Hornstrup ch@csh-consult.dk wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
Casper
_______________________________________________ Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Casper Hornstrup wrote:
Oh please don’t go there. Do you honestly think you (in general) are smarter at generating microprocessor instructions than a multi-million hour compiler software?
In some places it does matter, especially in the kernel.
Splitting up a 200 lines functions in 10 helper functions IMO is ugly, especially when those helper functions are only used by that one single function. To avoid code duplication I *do* create helper functions, but most often only for things that can be shared with other code. I don't have problems with functions with a few hundreds of lines of code as long as the coding style is acceptable and it is sufficiently documented with comments.
However, we're going off topic...
- Thomas
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of Thomas Weidenmueller Sent: 28. september 2005 18:15 To: ReactOS Development List Subject: Re: [ros-dev] Re:[ros-svn] [gdalsnes]18113:-reorderInsertXscendingOrdermacro argument order andupdate uses
Casper Hornstrup wrote:
Oh please don't go there. Do you honestly think you (in general) are smarter at generating microprocessor instructions than a multi-million hour compiler software?
In some places it does matter, especially in the kernel.
Prove it.
Splitting up a 200 lines functions in 10 helper functions IMO is ugly, especially when those helper functions are only used by that one single function. To avoid code duplication I *do* create helper functions, but most often only for things that can be shared with other code. I don't have problems with functions with a few hundreds of lines of code as long as the coding style is acceptable and it is sufficiently documented with comments.
However, we're going off topic...
- Thomas
Ok. I'll not argument against 200 line long functions then since it seems I won't get anywhere.
Casper
Thomas Weidenmueller wrote:
Casper Hornstrup wrote:
Oh please don’t go there. Do you honestly think you (in general) are smarter at generating microprocessor instructions than a multi-million hour compiler software?
In some places it does matter, especially in the kernel.
Splitting up a 200 lines functions in 10 helper functions IMO is ugly, especially when those helper functions are only used by that one single function. To avoid code duplication I *do* create helper functions, but most often only for things that can be shared with other code. I don't have problems with functions with a few hundreds of lines of code as long as the coding style is acceptable and it is sufficiently documented with comments.
However, we're going off topic...
I think you are on topic. These 10 helper functions usually make you avoid the gotos or the defines. One mans ugly is anothers beauty. Are 10 functions instead of one ugly? Are macros? Are gotos?
regards, Jakob
Casper Hornstrup wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
I agree with Nathan. Having tons of small functions often isn't a good solution, especially when you'd have to create dozens of small helper functions. That not just only generates slower code but also makes it more difficult to get a picture of the algorithm used. I much more prefer jumping to cleanup labels the way Nathan demonstrated it. Of course I avoid it where it doesn't make sense.
- Thomas
However an large function can also ramper readability and mantainiblility. I think the best thing is to evaluate on how to write a function on a case by case basis.
On 9/28/05, Thomas Weidenmueller w3seek@reactos.com wrote:
Casper Hornstrup wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
I agree with Nathan. Having tons of small functions often isn't a good solution, especially when you'd have to create dozens of small helper functions. That not just only generates slower code but also makes it more difficult to get a picture of the algorithm used. I much more prefer jumping to cleanup labels the way Nathan demonstrated it. Of course I avoid it where it doesn't make sense.
- Thomas
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
-- <P>My DeviantArt.com page: <A href="http://crashfourit.deviantart.com/"> http://crashfourit.deviantart.com/</A><BR>My FanFiction.net bio page: <A href="http://www.fanfiction.net/u/726606/"> http://www.fanfiction.net/u/726606/</A><BR>My Blog: <A href=" http://crashfourit.blogspot.com">http://crashfourit.blogspot.com</A><BR>America's Debate: <A href="http://www.americasdebate.com/"> http://www.americasdebate.com/</A> </P>
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of Thomas Weidenmueller Sent: 28. september 2005 17:27 To: ReactOS Development List Subject: Re: [ros-dev] Re:[ros-svn] [gdalsnes] 18113:-reorderInsertXscendingOrdermacro argument order and update uses
Casper Hornstrup wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
I agree with Nathan. Having tons of small functions often isn't a good solution, especially when you'd have to create dozens of small helper functions. That not just only generates slower code but also makes it more difficult to get a picture of the algorithm used. I much more prefer jumping to cleanup labels the way Nathan demonstrated it. Of course I avoid it where it doesn't make sense.
- Thomas
There is practically no difference in execution speed.
Casper
Take a look at the code below. More readable by far IMHO.
Casper Hornstrup wrote:
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] On Behalf Of Thomas Weidenmueller Sent: 28. september 2005 17:27 To: ReactOS Development List Subject: Re: [ros-dev] Re:[ros-svn] [gdalsnes] 18113:-reorderInsertXscendingOrdermacro argument order and update uses
Casper Hornstrup wrote:
The alternative is: do the cleanup at every return, use goto or use try/finally. 1)Cleanup at every return is madness. Most functions in ros do a large amount of cleanup at each return and I sometimes spot mistakes where one/more return misses some cleanup. Those errors are _hard_ to find.
The functions are too large then. Use more smaller functions instead.
I agree with Nathan. Having tons of small functions often isn't a good solution, especially when you'd have to create dozens of small helper functions. That not just only generates slower code but also makes it more difficult to get a picture of the algorithm used. I much more prefer jumping to cleanup labels the way Nathan demonstrated it. Of course I avoid it where it doesn't make sense.
- Thomas
There is practically no difference in execution speed.
Casper
#include <stdio.h> #include <windows.h>
#define NumberOfIterations 60000
void RunCollapsed() { int i, j; for (i = 0; i < NumberOfIterations; i++) { for (j = 0; j < NumberOfIterations; j++) { } } }
static void Helper() { int j; for (j = 0; j < NumberOfIterations; j++) { } }
void RunNonCollapsed() { int i; for (i = 0; i < NumberOfIterations; i++) { Helper(); } }
void RunOnce(LARGE_INTEGER *collapsedTicks, LARGE_INTEGER *nonCollapsedTicks) { LARGE_INTEGER start; LARGE_INTEGER stop;
QueryPerformanceCounter(&start); RunCollapsed(); QueryPerformanceCounter(&stop); collapsedTicks->QuadPart = stop.QuadPart - start.QuadPart; printf("Collapsed: %lu ticks\n", collapsedTicks->QuadPart);
QueryPerformanceCounter(&start); RunNonCollapsed(); QueryPerformanceCounter(&stop); nonCollapsedTicks->QuadPart = stop.QuadPart - start.QuadPart; printf("NonCollapsed: %lu ticks\n", nonCollapsedTicks->QuadPart);
printf("%d%%\n", (nonCollapsedTicks->QuadPart * 100) / collapsedTicks->QuadPart); }
int main() { LARGE_INTEGER collapsedTicks; LARGE_INTEGER nonCollapsedTicks; LARGE_INTEGER totalCollapsedTicks; LARGE_INTEGER totalNonCollapsedTicks; int i;
totalCollapsedTicks.QuadPart = 0; totalNonCollapsedTicks.QuadPart = 0; for (i = 0; i < 10; i++) { RunOnce(&collapsedTicks, &nonCollapsedTicks); totalCollapsedTicks.QuadPart += collapsedTicks.QuadPart; totalNonCollapsedTicks.QuadPart += nonCollapsedTicks.QuadPart; } printf("Average: %d%%\n", (totalNonCollapsedTicks.QuadPart * 100) / totalCollapsedTicks.QuadPart); }
Ros-dev mailing list Ros-dev@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
Gunnar Dalsnes wrote:
Phillip Susi wrote:
Recently I was reading over some apache code and they make extensive use of macros for flow control. I found myself becoming frustrated to no end because I could not figure out what the hell was going on. They looked like nice, neat function calls, but I could not for the life of me figure out where the code was that was being called.
So if they had been function calls and not macros it would have been easier??? Or should we stop using functions also? ;-P
Well, yes...
You can't step into macros in most debuggers (that I use.)
As Alex mentioned, it is a pain to try and sprinkle debugging statements in a macro (if only because the syntax is so ugly.)
Macros have ill defined side effects. This is especially damaging to code maintainability when macros look like functions.
Macros are bug prone because they have additional unique issues over normal code, they are hard to read, and hard to debug.
C++ (and some latter version of C if I'm not mistaken) added inline functions for a reason: so programmers would stop using macros.
If you cant find a macro (simple text search) you probably wouldnt have understod the code _without_ the macros either;-P
If I'm searching for something that looks like a function call (especially if I'm searching for the definition not the declaration), I might be looking in the wrong places or searching using syntax that doesn't catch a #define.
Thanks,
Joseph
Gunnar Dalsnes wrote:
In win32k i use macros for flow control in the NtUser syscalls (functionally like a try/finally block) where its very important that some cleanup (release a lock) is always done:
BOOL NtFunc() { DECLARE_RETURN(BOOL);
Lock(); if (Stuff) RETURN(FALSE); .... RETURN(TRUE);
CLEANUP: Unlock(Stuff); DPRINT1("NtFunc returned %i\n", _ret_); END_CLEANUP; }
Patterns like this can be done without macros. I've always been partial to the following style:
BOOL NtFunc() { BOOL bResult; void *pPointer = NULL;
Lock();
if (Stuff) { bResult = FALSE; goto cleanup; } ....
bResult = TRUE;
cleanup: if (pPointer) free(pPointer); Unlock(stuff); DPRINT1("NtFunc returned %i\n", bResult); return bResult; }
I know that many find goto's to be a crime against humanity, but I've always favored them for this sort of pattern.
-N
Gunnar Dalsnes wrote:
In win32k i use macros for flow control in the NtUser syscalls (functionally like a try/finally block) where its very important that some cleanup (release a lock) is always done:
BOOL NtFunc() { DECLARE_RETURN(BOOL);
Lock(); if (Stuff) RETURN(FALSE); .... RETURN(TRUE);
CLEANUP: Unlock(Stuff); DPRINT1("NtFunc returned %i\n", _ret_); END_CLEANUP; }
This is EXACTLY my point. I intended to make macros much like that to handle error recovery and cleanup. The problem is that it is very hard to understand. It is much more clear to have something like this:
NTSTATUS NtFunc() { Status = DoSomething(); if( !NT_SUCCESS( Status ) ) goto ret; Status = DoSomething2(); if( !NT_SUCCESS( Status ) ) goto cleanup1; Status = DoSomething3(); if( !NT_SUCCESS( Status ) ) goto cleanup2;
... etc
cleanup2: Cleanup2(); cleanup1: Cleanup1(); ret: return Status; }
At first it is temping to use macros to get rid of the duplicate if( !NTSUCCESS... lines, but anyone who knows C can read that code. With the macros, they must first figure out how the macros work, which is often not very simple. The most important attribute code can have is maintainability, and readability and simplicity lead to maintainability. I finally realized that the real reason I wanted to make macros was because I was sick of retyping the same line of code over and over, but the readability of the code is far more important than saving the writer a few keystrokes.
Your example code is one of the reasons that C++ developed exceptions. People kept writing code that was death by macros exactly that way for exactly that reason, so the developers of C++ decided it was a good idea to put support directly into the language for the needed functionality, without the use of macros.
If we should only invent stuff that ppl _already_ understand we just had to stop inventing.
Inventing for the sake of inventing is a bad thing. We invent things because they are useful. When you invent things that change the language to the point that another person who is fluent in the language can not understand what you are saying, that is not useful.
Recently I was reading over some apache code and they make extensive use of macros for flow control. I found myself becoming frustrated to no end because I could not figure out what the hell was going on. They looked like nice, neat function calls, but I could not for the life of me figure out where the code was that was being called.
So if they had been function calls and not macros it would have been easier??? Or should we stop using functions also? ;-P
Yes, because with a function call it is immediately apparent to anyone who knows C where the flow of code goes. I spent some time trying to understand the flow of code there and finally gave up. The macros were so complex that I couldn't figure them out in a reasonable amount of time, so I could not follow the flow of the code, and hence, was unable to maintain that code.
If you cant find a macro (simple text search) you probably wouldnt have understod the code _without_ the macros either;-P
It isn't a matter of finding the macro. Taking valuable time out from reading already complex code to go find a complex macro, then try to parse it and figure out what the real code you were looking at gets expanded to for the compiler, then figre out what the expanded code actually does can get very complex very quickly. Remember, the number of bugs grows exponentially with the complexity of the system, so anything one can do to reduce complexity is a very good thing.
But off course you have to learn how the macros work to understand and utilize them, but that goes for any code. I cant see how learning a function call with 10+ params (typical nt syscall func) should be any easier than learning a simple macro. Thats just being lazy.
G.
It is easier because when you see a function call you immediately know exactly where the code flows to. It flows to the function that has that name, with the parameters listed, and then back again. No more, and no less. You don't have to figure out if it ends up passing control to three other places you don't know about, and if control eventually returns to this point or not. You can easily find the called function which should have a comment block at the start explaining what the parameters mean in plain english. Usually no further reading is required. With a macro you have to find the macro, parse the terse SOB, go back to the code you were trying to understand in the first place, figure out how the compiler actually sees that code after the macro gets ahold of it, and then figure out what THAT code does. It adds a lot of work with no real benefit.
Gunnar Dalsnes wrote:
If you come across a crash during an enum you just temporarily change the code to hunt the bug, just like normal bug hunting. Some of the list enums i changed were on this form...
No, not like during normal bug hunting. Durnig normal bug hunting I add some dprints. During macro bug-hunting I revert the code to make it stop using the macro (potentially making a mistake and introducing a bug), add the debug prints, get it working, then attempt to add the macro again (and potentially make a mistake). More work for me, and more potential for bugs.
for (HeadList = &Foo.Flink; Entry != HeadList; Entry = Entry->Flink) { Object = CONTAINING_RECORD(Entry, OBJECT, Link); Object->stuff }
...and if you had to "debug" it like you showed youd had to edit the code just as well.
That's because that loop is really ugly. I don't think I'd ever write a loop like that for manipulating list entries.
But for normal not-in-process-of-being-debugged-code i think the above code sux (even without the dprints).
Yeah it does, it should be expanded to properly show each step line by line. Not macroized into a single unmaintainable ..thing.
Best regards, Alex Ionescu
Yeah it does, it should be expanded to properly show each step line by line.
Thats your opinion. Seeing the boring internals of walking a list a billion times only pisses me off. I rather hide it where it cant be seen and focus on the usefull code instead.
Not macroized into a single unmaintainable ..thing.
There really is no way of "maintaining" how to walk a list. This is pretty much established "science". That why a macro is nice, because i dont want you to see it nor maintain it (or comment on the basic operations of list walking);-P
G.
Gunnar Dalsnes wrote:
Yeah it does, it should be expanded to properly show each step line by line.
Thats your opinion. Seeing the boring internals of walking a list a billion times only pisses me off. I rather hide it where it cant be seen and focus on the usefull code instead.
Well, especially because it's the kernel, I agree with Alex. It shouln't be hidden. I strongly prefer it the way it used to be.
Not macroized into a single unmaintainable ..thing.
There really is no way of "maintaining" how to walk a list. This is pretty much established "science". That why a macro is nice, because i dont want you to see it nor maintain it (or comment on the basic operations of list walking);-P
Sometimes you want a loop to be slightly different. That's where the macro fails. However, f.ex. inserting a list item or removing it is _always_ the same, that's why there are macros for these and MS would've defined such a macro if it was such a nice idea.
I can only reiterate, I'm never going to use these macros. At least not in ntoskrnl or any drivers.
- Thomas
On Tuesday 27 September 2005 01:14, Alex Ionescu wrote:
gdalsnes@svn.reactos.com wrote:
-reorder InsertXscendingOrder macro argument order and update uses -start to macrofy list enums in ntoskrnl using LIST_FOR_EACH macros -use IsListEmpty some places instead of manual coding -profile.c:KeStartProfile() zero correct buffer in (and not the NULL ptr;-) -improve LIST_FOR_EACH macros so that the iterator is set to NULL at end of enum -kthread.c:KiServiceCheck() set the shadowtable before calling the win32k proc/thread init funcs -apc.c:KiInsertQueueApc() insert special apcs in correct fifo order -apc.c:KeFlushQueueApc() simplify -timer.c:KiExpireTimers() calling RemoveEntryList on a possibly empty list is not ok
Updated files: trunk/reactos/include/reactos/helper.h trunk/reactos/ntoskrnl/io/device.c trunk/reactos/ntoskrnl/io/driver.c trunk/reactos/ntoskrnl/io/file.c trunk/reactos/ntoskrnl/io/fs.c trunk/reactos/ntoskrnl/io/irp.c trunk/reactos/ntoskrnl/io/pnpnotify.c trunk/reactos/ntoskrnl/io/pnproot.c trunk/reactos/ntoskrnl/ke/apc.c trunk/reactos/ntoskrnl/ke/bug.c trunk/reactos/ntoskrnl/ke/kqueue.c trunk/reactos/ntoskrnl/ke/kthread.c trunk/reactos/ntoskrnl/ke/profile.c trunk/reactos/ntoskrnl/ke/queue.c trunk/reactos/ntoskrnl/ke/timer.c trunk/reactos/subsys/system/usetup/partlist.c trunk/reactos/subsys/win32k/ntuser/class.c
Hi,
I'm going to have to voice my extreme outcry against this patch. It is replacing clear and easily debuggable code by non-standard macros which are impossible to debug. It is also going to create problems when people which are not used to them (such as myself):
- Have no idea what some LIST_FOR_EACH_SAFE thing is doing vs
LIST_FOR_EACH vs InsertListLifoSafeMaybeLoopOrWhoKnowsWhat 2) Will never code using these macros, and thus the source will become full of both methods, which is totally ugly.
Sometimes, especially in kernel code, it's much better to have an expanded logic then to use a macro. I wrote a great majority of the code that has just been, imho, pollutted with these macros, and I don't appreciate for someone to barge in and change my code for some macro that I don't understand, nor wish to, because it only causes problems. I take great care of the code I write and "own" it (in the programming term) so that if a bug in it arises, I can quickly identify it and be held responsible for it. The addition of these macros hinders that effort.
I don't mind someone using them inside his or her own code. I just mind their usage in ntoskrnl, and even more so when it overwrites the clean, clear code that I've written and am used to debugging.
Best regards, Alex Ionescu
You know, it also sucked when you changed the syntax of all the assembly files in ntoskrnl, thats when i stopped working on it. So, are you the only one who is allowed to do such changes because you believe to be the best?
-
Alex Ionescu -
Bryan Quigley -
Casper Hornstrup -
crashfourit -
crashofurit
-
Ge van Geldorp -
Gregor Anich -
Gunnar Dalsnes -
Jakob Eriksson -
Joseph Galbraith -
KJKHyperion -
Nathan Woods -
Phillip Susi -
Richard Campbell -
Rick Langschultz -
Steven Edwards -
Thomas Weidenmueller -
TwoTailedFox