Re: dlls/kernel32/task.c -- minor improvement
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
Index: dlls/kernel32/task.c =================================================================== RCS file: /home/wine/wine/dlls/kernel32/task.c,v retrieving revision 1.2 diff -u -3 -p -r1.2 task.c --- dlls/kernel32/task.c 13 Oct 2006 10:27:19 -0000 1.2 +++ dlls/kernel32/task.c 3 Jan 2008 21:33:44 -0000 @@ -57,7 +57,7 @@ typedef struct WORD magic; /* Thunks signature */ WORD unused; WORD free; /* Head of the free list */ - WORD thunks[4]; /* Each thunk is 4 words long */ + WORD thunks[]; /* Each thunk is 4 words long */ } THUNKS;
That syntax is not portable. -- Alexandre Julliard julliard(a)winehq.org
On Fri, 4 Jan 2008, Alexandre Julliard wrote:
- WORD thunks[4]; /* Each thunk is 4 words long */ + WORD thunks[]; /* Each thunk is 4 words long */ That syntax is not portable.
That depends. ;-) You're right, it is not ISO C90, but it is in the C99 standard which is now close to ten years old. % gcc -pedantic x.c -c -o x -std=c99 % gcc -pedantic x.c -c -o x -std=c89 x.c:3: warning: ISO C90 does not support flexible array members % gcc -pedantic x.c -c -o x x.c:3: warning: ISO C90 does not support flexible array members Which compiler that we care about doesn't support this C99 feature? That said, if you don't want to take the original patch, can we take the one below? Thanks, Gerald ChangeLog: Use 4*sizeof(WORD) instead of the magic constant 8 for THUNKS.thunks, and explain why we cannot use an open array type THUNKS.thunks[]. Index: dlls/kernel32/task.c =================================================================== RCS file: /home/wine/wine/dlls/kernel32/task.c,v retrieving revision 1.2 diff -u -3 -p -r1.2 task.c --- dlls/kernel32/task.c 13 Oct 2006 10:27:19 -0000 1.2 +++ dlls/kernel32/task.c 6 Jan 2008 12:26:32 -0000 @@ -57,7 +57,10 @@ typedef struct WORD magic; /* Thunks signature */ WORD unused; WORD free; /* Head of the free list */ - WORD thunks[4]; /* Each thunk is 4 words long */ + WORD thunks[4]; /* Each thunk is 4 words long, and while we will + * dynamically allocate the whole struct according + * to the actual number of thunks we cannot use the + * thunks[] syntax here because it is not C90. */ } THUNKS; #include "poppack.h" @@ -174,7 +177,7 @@ static void TASK_CreateThunks( HGLOBAL16 free = pThunk->free; for (i = 0; i < count-1; i++) { - free += 8; /* Offset of next thunk */ + free += 4*sizeof(WORD); /* Offset of next thunk */ pThunk->thunks[4*i] = free; } pThunk->thunks[4*i] = 0; /* Last thunk */ @@ -201,7 +204,8 @@ static SEGPTR TASK_AllocThunk(void) sel = pThunk->next; if (!sel) /* Allocate a new segment */ { - sel = GLOBAL_Alloc( GMEM_FIXED, sizeof(THUNKS) + (MIN_THUNKS-1)*8, + sel = GLOBAL_Alloc( GMEM_FIXED, + sizeof(THUNKS) + (MIN_THUNKS-1)*4*sizeof(WORD), pTask->hPDB, WINE_LDT_FLAGS_CODE ); if (!sel) return (SEGPTR)0; TASK_CreateThunks( sel, 0, MIN_THUNKS );
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
That depends. ;-) You're right, it is not ISO C90, but it is in the C99 standard which is now close to ten years old.
We don't use C99 features in Wine.
That said, if you don't want to take the original patch, can we take the one below?
I don't think that's an improvement, thunks are not really words, they are code. -- Alexandre Julliard julliard(a)winehq.org
On Sun, 6 Jan 2008, Alexandre Julliard wrote:
We don't use C99 features in Wine.
Actually we do. % egrep -r "^(static )?inline " $WINE_SOURCE/ | wc -l 2031 "inline" is pretty similar to flexible array members in that both were not in C90, both are in C99, and both have been GNU extensions before C99, so I'm not sure why we use one but not the other.
That said, if you don't want to take the original patch, can we take the one below? I don't think that's an improvement, thunks are not really words, they are code.
I know, but in the current code we do have WORD thunks[4] and later on we twice relyy on the magic constant 8 which comes from that (4*sizeof WORD). Honestly, I'd hope my patch makes this code easier to grasp. It certainly would have helped *me* when first looking at this code! ;-) Gerald ChangeLog: Use 4*sizeof(WORD) instead of the magic constant 8 for THUNKS.thunks, and explain why we cannot use an open array type THUNKS.thunks[]. Index: dlls/kernel32/task.c =================================================================== RCS file: /home/wine/wine/dlls/kernel32/task.c,v retrieving revision 1.2 diff -u -3 -p -r1.2 task.c --- dlls/kernel32/task.c 13 Oct 2006 10:27:19 -0000 1.2 +++ dlls/kernel32/task.c 6 Jan 2008 12:26:32 -0000 @@ -57,7 +57,10 @@ typedef struct WORD magic; /* Thunks signature */ WORD unused; WORD free; /* Head of the free list */ - WORD thunks[4]; /* Each thunk is 4 words long */ + WORD thunks[4]; /* Each thunk is 4 words long, and while we will + * dynamically allocate the whole struct according + * to the actual number of thunks we cannot use the + * thunks[] syntax here because it is not C90. */ } THUNKS; #include "poppack.h" @@ -174,7 +177,7 @@ static void TASK_CreateThunks( HGLOBAL16 free = pThunk->free; for (i = 0; i < count-1; i++) { - free += 8; /* Offset of next thunk */ + free += 4*sizeof(WORD); /* Offset of next thunk */ pThunk->thunks[4*i] = free; } pThunk->thunks[4*i] = 0; /* Last thunk */ @@ -201,7 +204,8 @@ static SEGPTR TASK_AllocThunk(void) sel = pThunk->next; if (!sel) /* Allocate a new segment */ { - sel = GLOBAL_Alloc( GMEM_FIXED, sizeof(THUNKS) + (MIN_THUNKS-1)*8, + sel = GLOBAL_Alloc( GMEM_FIXED, + sizeof(THUNKS) + (MIN_THUNKS-1)*4*sizeof(WORD), pTask->hPDB, WINE_LDT_FLAGS_CODE ); if (!sel) return (SEGPTR)0; TASK_CreateThunks( sel, 0, MIN_THUNKS );
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
On Sun, 6 Jan 2008, Alexandre Julliard wrote:
We don't use C99 features in Wine.
Actually we do.
% egrep -r "^(static )?inline " $WINE_SOURCE/ | wc -l 2031
"inline" is pretty similar to flexible array members in that both were not in C90, both are in C99, and both have been GNU extensions before C99, so I'm not sure why we use one but not the other.
inline is automatically disabled if the compiler doesn't support it. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
Index: dlls/kernel32/task.c =================================================================== RCS file: /home/wine/wine/dlls/kernel32/task.c,v retrieving revision 1.2 diff -u -3 -p -r1.2 task.c --- dlls/kernel32/task.c 13 Oct 2006 10:27:19 -0000 1.2 +++ dlls/kernel32/task.c 3 Jan 2008 21:33:44 -0000 @@ -57,7 +57,7 @@ typedef struct WORD magic; /* Thunks signature */ WORD unused; WORD free; /* Head of the free list */ - WORD thunks[4]; /* Each thunk is 4 words long */ + WORD thunks[]; /* Each thunk is 4 words long */ } THUNKS;
That syntax is not portable.
We have the ANYSIZE_ARRAY macro in include/winnt.h for this exact purpose. -- Rob Shearman
participants (3)
-
Alexandre Julliard -
Gerald Pfeifer -
Robert Shearman