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.
30 Jun
2006
30 Jun
'06
5:53 p.m.
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,
30 Jun
30 Jun
7:14 p.m.
New subject: [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.
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 
10:47 p.m.
New subject: [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.
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(a)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(a)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(a)svn.reactos.org wrote:
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
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
6716
days inactive
6716
days old
2 comments
3 participants
participants (3)
-
Maarten Bosma -
Peter Dolding -
Saveliy Tretiakov