[ros-dev] [ros-diffs] [greatlrd] 22718: Small clean up 1. Remove goto in the code, goto is slow and should be avoid. 2. reformat for adding {} around some code. 3. remove some NULL check after I did remove goto that is not longer need it.
Peter Dolding
oiaohm at bluebottle.com
Sat Jul 1 00:47:45 CEST 2006
Person has to learn its a balance.
Goto's are not slow common miss understanding OOP programmers. It
depends on the size of the jump. Slowest minorly faster than a function
call.
In speed critical sections of the Linux kernel you will find Goto's to
avoid function calls and reduced memory usage.
OOP vs Spaghetti/*. */It looks like to me. OOP is slower most of the
time.
OOP most of the time is simpler to look after. OOP does not use Goto's.
There are cases where Spaghetti is neater. As well has cases where its
faster or smaller.
The alteration has made the code a little faster. But its swaping speed
for size. The new one is larger and will use more memory in operation.
If you where using a OOP workaround to keep size down. The section
pointed out by Savality would have been in a function. Maybe a function
inside function.
int somefunction() {
void internalfunc(){
};
}
This is slower than using the goto's. This section has to have a
judgment call. Is the expanded size worth it for the speed
improvement. It might be a critical section where bloat is important.
Even so the internalfunc() could have had a inline added in that case
allowing about the same speed and size as greatlrd at svn.reactos.org but
neater code to look after and maintain. Also the code inlined code will
respond to Size or Speed flag of the complier on how it should be
handled. Its making it simpler for the complier to alter the file as well.
Sorry to say greatlrd at svn.reactos.org. The alteration does not obey
either OOP or Speghetti for readable compact maintainable code. The
version before you touched it obeyed Compact Maintainable Speghetti.
Yes a lot of OOP programmers don't partially like it because they keep
on being told that goto's are bad. They are bad if they are the only
thing you use. Yet as bad as it sounds a program built completely
without internal functions using just goto calls is faster but a real
nightmare to debug and work on yet can beat OOP code doing the same
thing on speed without correctly placed inlines. Even so the Speghetti
will be smaller than the inlined OOP yet almost as fast. Its one of the
reasons why a asm programmer can build a smaller program than a C
programmer. They don't fear the goto's I would not call good asm
partially maintainable.
I don't know that section well enough to make a call on what way it
should be. Small or Really Really fast and slightly more memory using.
If section
Maarten Bosma wrote:
> 1. Are gotos really slow ? Arn't they just translated into a jmp ?
> 2. Why do you reformat foreign code ?
>
> Maarten Bosma
>
> Saveliy Tretiakov wrote:
>
>> I don't like your changes to main(), you added a lot of code
>> dublication, made it harder to read and maintain. This is repeated three
>> times:
>> DeleteCriticalSection(&LogListCs);
>>
>> // Close all log files.
>> for(pLogf = LogListHead; pLogf; pLogf = ((PLOGFILE)pLogf)->Next)
>> {
>> LogfClose(pLogf);
>> }
>>
>> HeapDestroy(MyHeap);
>>
>> greatlrd at svn.reactos.org wrote:
>>
>>
>>> Author: greatlrd
>>> Date: Fri Jun 30 20:42:21 2006
>>> New Revision: 22718
>>>
>>> URL: http://svn.reactos.org/svn/reactos?rev=22718&view=rev
>>> Log:
>>> Small clean up
>>> 1. Remove goto in the code, goto is slow and should be avoid.
>>> 2. reformat for adding {} around some code.
>>> 3. remove some NULL check after I did remove goto that is not longer need it.
>>>
>>>
>>> Modified:
>>> trunk/reactos/base/services/eventlog/eventlog.c
>>>
>>> Modified: trunk/reactos/base/services/eventlog/eventlog.c
>>> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/eventlog/eventlog.c?rev=22718&r1=22717&r2=22718&view=diff
>>> ==============================================================================
>>> --- trunk/reactos/base/services/eventlog/eventlog.c (original)
>>> +++ trunk/reactos/base/services/eventlog/eventlog.c Fri Jun 30 20:42:21 2006
>>> @@ -61,7 +61,7 @@
>>> DWORD MaxValueLen, ValueLen, Type, ExpandedLen;
>>> WCHAR *Buf = NULL, *Expanded = NULL;
>>> LONG Result;
>>> - BOOL ret = FALSE;
>>> + BOOL ret = TRUE;
>>> PLOGFILE pLogf;
>>>
>>> DPRINT("LoadLogFile: %S\n", LogName);
>>> @@ -88,12 +88,14 @@
>>> if(Result != ERROR_SUCCESS)
>>> {
>>> DPRINT1("RegQueryValueEx failed: %d\n", GetLastError());
>>> - goto cleanup;
>>> + HeapFree(MyHeap, 0, Buf);
>>> + return FALSE;
>>> }
>>> if(Type != REG_EXPAND_SZ && Type != REG_SZ)
>>> {
>>> DPRINT1("%S\\File - value of wrong type %x.\n", LogName, Type);
>>> - goto cleanup;
>>> + HeapFree(MyHeap, 0, Buf);
>>> + return FALSE;
>>> }
>>>
>>> ExpandedLen = ExpandEnvironmentStrings(Buf, NULL, 0);
>>> @@ -102,7 +104,8 @@
>>> if(!Expanded)
>>> {
>>> DPRINT1("Can't allocate heap!\n");
>>> - goto cleanup;
>>> + HeapFree(MyHeap, 0, Buf);
>>> + return FALSE;
>>> }
>>>
>>> ExpandEnvironmentStrings(Buf, Expanded, ExpandedLen);
>>> @@ -114,14 +117,11 @@
>>> if(pLogf == NULL)
>>> {
>>> DPRINT1("Failed to create %S!\n", Expanded);
>>> - goto cleanup;
>>> - }
>>> -
>>> - ret = TRUE;
>>> -
>>> -cleanup:
>>> + ret = FALSE;
>>> + }
>>> +
>>> HeapFree(MyHeap, 0, Buf);
>>> - if(Expanded) HeapFree(MyHeap, 0, Expanded);
>>> + HeapFree(MyHeap, 0, Expanded);
>>> return ret;
>>> }
>>>
>>> @@ -129,8 +129,7 @@
>>> {
>>> LONG result;
>>> DWORD MaxLognameLen, LognameLen;
>>> - WCHAR *Buf = NULL;
>>> - BOOL ret = FALSE;
>>> + WCHAR *Buf = NULL;
>>> INT i;
>>>
>>> RegQueryInfoKey(eventlogKey, NULL, NULL, NULL, NULL, &MaxLognameLen,
>>> @@ -143,7 +142,7 @@
>>> if(!Buf)
>>> {
>>> DPRINT1("Error: can't allocate heap!\n");
>>> - return ret;
>>> + return FALSE;
>>> }
>>>
>>> i = 0;
>>> @@ -159,7 +158,8 @@
>>> if(result != ERROR_SUCCESS)
>>> {
>>> DPRINT1("Failed to open %S key.\n", Buf);
>>> - goto cleanup;
>>> + HeapFree(MyHeap, 0, Buf);
>>> + return FALSE;
>>> }
>>>
>>> if(!LoadLogFile(SubKey, Buf))
>>> @@ -171,18 +171,14 @@
>>> i++;
>>> }
>>>
>>> - ret = TRUE;
>>> -
>>> -cleanup:
>>> HeapFree(MyHeap, 0, Buf);
>>> - return ret;
>>> + return TRUE;
>>> }
>>>
>>> INT main()
>>> {
>>> WCHAR LogPath[MAX_PATH];
>>> - PLOGFILE pLogf;
>>> - INT RetCode = 0;
>>> + PLOGFILE pLogf;
>>> LONG result;
>>> HKEY elogKey;
>>>
>>> @@ -193,8 +189,15 @@
>>> if(MyHeap==NULL)
>>> {
>>> DPRINT1("FATAL ERROR, can't create heap.\n");
>>> - RetCode = 1;
>>> - goto bye_bye;
>>> +
>>> + DeleteCriticalSection(&LogListCs);
>>> + // Close all log files.
>>> + for(pLogf = LogListHead; pLogf; pLogf = ((PLOGFILE)pLogf)->Next)
>>> + {
>>> + LogfClose(pLogf);
>>> + }
>>> +
>>> + return 1;
>>> }
>>>
>>> GetWindowsDirectory(LogPath, MAX_PATH);
>>> @@ -214,8 +217,17 @@
>>> if(result != ERROR_SUCCESS)
>>> {
>>> DPRINT1("Fatal error: can't open eventlog registry key.\n");
>>> - RetCode = 1;
>>> - goto bye_bye;
>>> +
>>> + DeleteCriticalSection(&LogListCs);
>>> +
>>> + // Close all log files.
>>> + for(pLogf = LogListHead; pLogf; pLogf = ((PLOGFILE)pLogf)->Next)
>>> + {
>>> + LogfClose(pLogf);
>>> + }
>>> +
>>> + HeapDestroy(MyHeap);
>>> + return 1;
>>> }
>>>
>>> LoadLogFiles(elogKey);
>>> @@ -223,16 +235,16 @@
>>>
>>> StartServiceCtrlDispatcher(ServiceTable);
>>>
>>> -bye_bye:
>>> DeleteCriticalSection(&LogListCs);
>>>
>>> // Close all log files.
>>> for(pLogf = LogListHead; pLogf; pLogf = ((PLOGFILE)pLogf)->Next)
>>> + {
>>> LogfClose(pLogf);
>>> + }
>>>
>>> - if(MyHeap) HeapDestroy(MyHeap);
>>> -
>>> - return RetCode;
>>> + HeapDestroy(MyHeap);
>>> + return 0;
>>> }
>>>
>>> VOID EventTimeToSystemTime(DWORD EventTime,
>>>
>>>
>>>
>>>
>>>
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
More information about the Ros-dev
mailing list