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.

[Maarten Bosma]

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(a)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/eve…
> > ==============================================================================
> > --- 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(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev