Re: [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]

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 [e-mail] 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 [e-mail].  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:
> Are gotos really slow ? Arn't they just translated into a jmp ?
> 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);
> >
> > [e-mail] 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
> > > Remove goto in the code, goto is slow and should be avoid.
> > > reformat for adding {} around some code.
> > > 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/even...
> > > ==============================================================================
> > > --- 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
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev