Hi,
I took out a part of the function SHFileOperationW and put it in a new function - please give any comments: is this the way to make this function more readable you want it? what is a better way of calling such a function?
regards,
Joris
? .patch.diff.swp ? backup-010320051808-pre-wine.tgz ? backup-010920052053-pre-wine.tgz ? backup-011120051329-pre-wine.tgz ? backup-011120051339-pre-wine.tgz ? backup-011220050008-pre-wine.tgz ? backup-123120041614-pre-wine.tgz ? badrefs ? description-pak ? doc-pak ? getrefs ? patch.diff ? ref ? refs ? versions ? wine-working.deb ? wine_cvs-041231-1_i386.deb ? wine_cvs-050103-1_i386.deb ? wine_cvs-050109-1_i386.deb ? wine_cvs-050111.noalsa-1_i386.deb ? wine_cvs-050112.release-1_i386.deb ? programs/notepad2 Index: dlls/shell32/shlfileop.c =================================================================== RCS file: /home/wine/wine/dlls/shell32/shlfileop.c,v retrieving revision 1.50 diff -u -r1.50 shlfileop.c --- dlls/shell32/shlfileop.c 23 Dec 2004 17:12:07 -0000 1.50 +++ dlls/shell32/shlfileop.c 13 Jan 2005 14:13:33 -0000 @@ -861,6 +861,40 @@ #define ERROR_SHELL_INTERNAL_FILE_NOT_FOUND 1026 #define HIGH_ADR (LPWSTR)0xffffffff
+static int WINAPI SHFileOperationW_delete( WIN32_FIND_DATAW *wfd,SHFILEOPSTRUCTW nFileOp, LPWSTR pFromFile,LPWSTR pTempFrom,HANDLE hFind) + +{ + LPWSTR lpFileName; + BOOL b_Mask = (NULL != StrPBrkW(&pFromFile[1], wWildcardChars)); + int retCode = 0; + do + { + lpFileName = wfd->cAlternateFileName; + if (!lpFileName[0]) + lpFileName = wfd->cFileName; + if (IsDotDir(lpFileName) || + ((b_Mask) && IsAttribDir(wfd->dwFileAttributes) && (nFileOp.fFlags & FOF_FILESONLY))) + continue; + SHFileStrCpyCatW(&pFromFile[1], lpFileName, NULL); + /* TODO: Check the SHELL_DeleteFileOrDirectoryW() function in shell32.dll */ + if (IsAttribFile(wfd->dwFileAttributes)) + { + if(SHNotifyDeleteFileW(pTempFrom) != ERROR_SUCCESS) + { + nFileOp.fAnyOperationsAborted = TRUE; + retCode = 0x78; /* value unknown */ + } + } + else if(!SHELL_DeleteDirectoryW(pTempFrom, (!(nFileOp.fFlags & FOF_NOCONFIRMATION)))) + { + nFileOp.fAnyOperationsAborted = TRUE; + retCode = 0x79; /* value unknown */ + } + } + while (!nFileOp.fAnyOperationsAborted && FindNextFileW(hFind,wfd)); + return retCode; +} + /************************************************************************* * SHFileOperationW [SHELL32.@] * @@ -1080,33 +1114,9 @@ /* ??? b_Mask = (!SHFileStrICmpA(&pFromFile[1], &wfd.cFileName[0], HIGH_ADR, HIGH_ADR)); */ if (!pTo) /* FO_DELETE */ { - do - { - lpFileName = wfd.cAlternateFileName; - if (!lpFileName[0]) - lpFileName = wfd.cFileName; - if (IsDotDir(lpFileName) || - ((b_Mask) && IsAttribDir(wfd.dwFileAttributes) && (nFileOp.fFlags & FOF_FILESONLY))) - continue; - SHFileStrCpyCatW(&pFromFile[1], lpFileName, NULL); - /* TODO: Check the SHELL_DeleteFileOrDirectoryW() function in shell32.dll */ - if (IsAttribFile(wfd.dwFileAttributes)) - { - if(SHNotifyDeleteFileW(pTempFrom) != ERROR_SUCCESS) - { - nFileOp.fAnyOperationsAborted = TRUE; - retCode = 0x78; /* value unknown */ - } - } - else - { - if(!SHELL_DeleteDirectoryW(pTempFrom, (!(nFileOp.fFlags & FOF_NOCONFIRMATION)))) - { - nFileOp.fAnyOperationsAborted = TRUE; - retCode = 0x79; /* value unknown */ - } - } - } while (!nFileOp.fAnyOperationsAborted && FindNextFileW(hFind, &wfd)); + int ret = SHFileOperationW_delete(&wfd,nFileOp,pFromFile,pTempFrom,hFind); + if (ret != 0) + retCode = ret; FindClose(hFind); hFind = INVALID_HANDLE_VALUE; if (nFileOp.fAnyOperationsAborted) Index: dlls/wined3d/basetexture.c =================================================================== RCS file: /home/wine/wine/dlls/wined3d/basetexture.c,v retrieving revision 1.2 diff -u -r1.2 basetexture.c --- dlls/wined3d/basetexture.c 9 Jan 2005 17:37:02 -0000 1.2 +++ dlls/wined3d/basetexture.c 13 Jan 2005 14:13:34 -0000 @@ -37,16 +37,17 @@
ULONG WINAPI IWineD3DBaseTextureImpl_AddRef(IWineD3DBaseTexture *iface) { IWineD3DBaseTextureImpl *This = (IWineD3DBaseTextureImpl *)iface; - TRACE("(%p) : AddRef increasing from %ld\n", This, This->resource.ref); + ULONG ref = InterlockedIncrement(&This->resource.ref); + + TRACE("(%p) : AddRef increasing from %ld\n", This,ref - 1); IUnknown_AddRef(This->resource.parent); - return InterlockedIncrement(&This->resource.ref); + return ref; }
ULONG WINAPI IWineD3DBaseTextureImpl_Release(IWineD3DBaseTexture *iface) { IWineD3DBaseTextureImpl *This = (IWineD3DBaseTextureImpl *)iface; - ULONG ref; - TRACE("(%p) : Releasing from %ld\n", This, This->resource.ref); - ref = InterlockedDecrement(&This->resource.ref); + ULONG ref = InterlockedDecrement(&This->resource.ref); + TRACE("(%p) : Releasing from %ld\n", This, ref + 1); if (ref == 0) { IWineD3DDevice_Release((IWineD3DDevice *)This->resource.wineD3DDevice); HeapFree(GetProcessHeap(), 0, This); Index: dlls/wined3d/indexbuffer.c =================================================================== RCS file: /home/wine/wine/dlls/wined3d/indexbuffer.c,v retrieving revision 1.4 diff -u -r1.4 indexbuffer.c --- dlls/wined3d/indexbuffer.c 9 Jan 2005 17:37:02 -0000 1.4 +++ dlls/wined3d/indexbuffer.c 13 Jan 2005 14:13:34 -0000 @@ -38,16 +38,16 @@
ULONG WINAPI IWineD3DIndexBufferImpl_AddRef(IWineD3DIndexBuffer *iface) { IWineD3DIndexBufferImpl *This = (IWineD3DIndexBufferImpl *)iface; - TRACE("(%p) : AddRef increasing from %ld\n", This, This->resource.ref); + ULONG ref = InterlockedIncrement(&This->resource.ref); + TRACE("(%p) : AddRef increasing from %ld\n", This, ref - 1); IUnknown_AddRef(This->resource.parent); - return InterlockedIncrement(&This->resource.ref); + return ref }
ULONG WINAPI IWineD3DIndexBufferImpl_Release(IWineD3DIndexBuffer *iface) { IWineD3DIndexBufferImpl *This = (IWineD3DIndexBufferImpl *)iface; - ULONG ref; - TRACE("(%p) : Releasing from %ld\n", This, This->resource.ref); - ref = InterlockedDecrement(&This->resource.ref); + ULONG ref = InterlockedDecrement(&This->resource.ref); + TRACE("(%p) : Releasing from %ld\n", This, ref + 1); if (ref == 0) { HeapFree(GetProcessHeap(), 0, This->allocatedMemory); IWineD3DDevice_Release((IWineD3DDevice *)This->resource.wineD3DDevice); Index: dlls/wined3d/resource.c =================================================================== RCS file: /home/wine/wine/dlls/wined3d/resource.c,v retrieving revision 1.4 diff -u -r1.4 resource.c --- dlls/wined3d/resource.c 9 Jan 2005 17:37:02 -0000 1.4 +++ dlls/wined3d/resource.c 13 Jan 2005 14:13:34 -0000 @@ -36,15 +36,15 @@
ULONG WINAPI IWineD3DResourceImpl_AddRef(IWineD3DResource *iface) { IWineD3DResourceImpl *This = (IWineD3DResourceImpl *)iface; - TRACE("(%p) : AddRef increasing from %ld\n", This, This->resource.ref); - return InterlockedIncrement(&This->resource.ref); + ULONG ref = InterlockedIncrement(&This->resource.ref); + TRACE("(%p) : AddRef increasing from %ld\n", This, ref - 1); + return ref; }
ULONG WINAPI IWineD3DResourceImpl_Release(IWineD3DResource *iface) { IWineD3DResourceImpl *This = (IWineD3DResourceImpl *)iface; - ULONG ref; - TRACE("(%p) : Releasing from %ld\n", This, This->resource.ref); - ref = InterlockedDecrement(&This->resource.ref); + ULONG ref = InterlockedDecrement(&This->resource.ref); + TRACE("(%p) : Releasing from %ld\n", This, ref + 1); if (ref == 0) { IWineD3DDevice_Release((IWineD3DDevice *)This->resource.wineD3DDevice); HeapFree(GetProcessHeap(), 0, This); Index: dlls/wined3d/surface.c =================================================================== RCS file: /home/wine/wine/dlls/wined3d/surface.c,v retrieving revision 1.1 diff -u -r1.1 surface.c --- dlls/wined3d/surface.c 9 Jan 2005 17:37:02 -0000 1.1 +++ dlls/wined3d/surface.c 13 Jan 2005 14:13:35 -0000 @@ -38,16 +38,16 @@
ULONG WINAPI IWineD3DSurfaceImpl_AddRef(IWineD3DSurface *iface) { IWineD3DSurfaceImpl *This = (IWineD3DSurfaceImpl *)iface; - TRACE("(%p) : AddRef increasing from %ld\n", This, This->resource.ref); + ULONG ref = InterlockedIncrement(&This->resource.ref); + TRACE("(%p) : AddRef increasing from %ld\n", This,ref - 1); IUnknown_AddRef(This->resource.parent); - return InterlockedIncrement(&This->resource.ref); + return ref }
ULONG WINAPI IWineD3DSurfaceImpl_Release(IWineD3DSurface *iface) { IWineD3DSurfaceImpl *This = (IWineD3DSurfaceImpl *)iface; - ULONG ref; - TRACE("(%p) : Releasing from %ld\n", This, This->resource.ref); - ref = InterlockedDecrement(&This->resource.ref); + ULONG ref = InterlockedDecrement(&This->resource.ref); + TRACE("(%p) : Releasing from %ld\n", This, ref + 1); if (ref == 0) { HeapFree(GetProcessHeap(), 0, This->allocatedMemory); IWineD3DDevice_Release((IWineD3DDevice *)This->resource.wineD3DDevice); Index: dlls/wined3d/vertexbuffer.c =================================================================== RCS file: /home/wine/wine/dlls/wined3d/vertexbuffer.c,v retrieving revision 1.6 diff -u -r1.6 vertexbuffer.c --- dlls/wined3d/vertexbuffer.c 9 Jan 2005 17:37:02 -0000 1.6 +++ dlls/wined3d/vertexbuffer.c 13 Jan 2005 14:13:35 -0000 @@ -38,16 +38,16 @@
ULONG WINAPI IWineD3DVertexBufferImpl_AddRef(IWineD3DVertexBuffer *iface) { IWineD3DVertexBufferImpl *This = (IWineD3DVertexBufferImpl *)iface; - TRACE("(%p) : AddRef increasing from %ld\n", This, This->resource.ref); + ULONG ref = InterlockedIncrement(&This->resource.ref); + TRACE("(%p) : AddRef increasing from %ld\n", This, ref - 1); IUnknown_AddRef(This->resource.parent); - return InterlockedIncrement(&This->resource.ref); + return ref; }
ULONG WINAPI IWineD3DVertexBufferImpl_Release(IWineD3DVertexBuffer *iface) { IWineD3DVertexBufferImpl *This = (IWineD3DVertexBufferImpl *)iface; - ULONG ref; - TRACE("(%p) : Releasing from %ld\n", This, This->resource.ref); - ref = InterlockedDecrement(&This->resource.ref); + ULONG ref = InterlockedDecrement(&This->resource.ref); + TRACE("(%p) : Releasing from %ld\n", This, ref + 1); if (ref == 0) { HeapFree(GetProcessHeap(), 0, This->allocatedMemory); IWineD3DDevice_Release((IWineD3DDevice *)This->resource.wineD3DDevice);
Joris Huizer wrote:
I took out a part of the function SHFileOperationW and put it in a new function - please give any comments: is this the way to make this function more readable you want it? what is a better way of calling such a function?
Looks like a step in the right direction. Anything that reduces the number of variables, side effects and conditions in that function would be appreciated.
Just be careful to make sure it still passes the test cases written for it...
Mike
Mike McCormack wrote:
Joris Huizer wrote:
I took out a part of the function SHFileOperationW and put it in a new function - please give any comments: is this the way to make this function more readable you want it? what is a better way of calling such a function?
Looks like a step in the right direction. Anything that reduces the number of variables, side effects and conditions in that function would be appreciated.
Just be careful to make sure it still passes the test cases written for it...
Mike
Uhm, trying to be sure all if functionally equivalent ofcourse - but how do I run test cases (I'm using linux here)
Joris
Joris Huizer wrote:
Uhm, trying to be sure all if functionally equivalent ofcourse - but how do I run test cases (I'm using linux here)
Make sure you have a valid .wine on your system, then run:
cd dlls/shell32 WINEPREFIX=~/.wine make test
You don't need to run them on Windows unless you're modifying the test itself. Somebody else already checked that they pass on various Windows versions.
It's also a good idea to try find a program that uses the function and test that too.
Mike
On Thu, 13 Jan 2005 15:12:17 +0100, Joris Huizer wrote:
I took out a part of the function SHFileOperationW and put it in a new function - please give any comments: is this the way to make this function more readable you want it? what is a better way of calling such a function?
Yes this looks good but for internal functions IMHO it's best to use GNU style naming, eg:
static int file_operation_delete(...) { }
That way it's (A) obvious what's internal and exported and (B) mixtures of GNU style and Win32 style in the same name are ugly :)
Remember internal functions don't need WINAPI linkage.
Finally be careful with diff, you included some Interlocked* fixes in there too.
thanks -mike
Mike Hearn wrote:
Yes this looks good but for internal functions IMHO it's best to use GNU style naming, eg:
static int file_operation_delete(...) { }
SHELL_internal_function() is a better way to go, IMO. It's already the Wine convention, you can easily locate the function, and it helps make sure no two functions will be named the same thing. There's no reason to make a new convention just because you think the old one is "ugly".
Mike
Mike McCormack wrote:
Mike Hearn wrote:
Yes this looks good but for internal functions IMHO it's best to use GNU style naming, eg:
static int file_operation_delete(...) { }
SHELL_internal_function() is a better way to go, IMO. It's already the Wine convention, you can easily locate the function, and it helps make sure no two functions will be named the same thing. There's no reason to make a new convention just because you think the old one is "ugly".
Mike
Hmm, I see this indeed; should I rename and send again? The convension seems to be like SHELL_internetFunction(), although there also seem to be a few SHELL32_* functions I also saw a few other debugger helper functions for SHFileOperationW, named debug_shfileops_flags and debug_shfileops_action; is this a special debugger convension or should those be renamed as well ?
Joris
Joris Huizer wrote:
Hmm, I see this indeed; should I rename and send again? The convension seems to be like SHELL_internetFunction(), although there also seem to be a few SHELL32_* functions
Up to you.
I also saw a few other debugger helper functions for SHFileOperationW, named debug_shfileops_flags and debug_shfileops_action; is this a special debugger convension or should those be renamed as well ?
Yeah, that was done by me in an attempt to copy the debugstr_w() and debugstd_guid() debugging helper functions, so it didn't follow the SHELL_ convention.
Mike
On Fri, 14 Jan 2005 01:57:29 +0900, Mike McCormack wrote:
SHELL_internal_function() is a better way to go, IMO. It's already the Wine convention, you can easily locate the function, and it helps make sure no two functions will be named the same thing. There's no reason to make a new convention just because you think the old one is "ugly".
Given the only reason we do this anymore is for etags/ctags I think I will start investigating whether it's possible for a tags implementation to restrict scope in tag searches. Because yes I do think the DLLNAME_ prefix we have all over the place is ugly and unnecessary ...
Mike McCormack wrote:
Mike Hearn wrote:
Yes this looks good but for internal functions IMHO it's best to use GNU style naming, eg:
static int file_operation_delete(...) { }
SHELL_internal_function() is a better way to go, IMO. It's already the Wine convention,
An ugly convention. SHOUTING is not nice to read when using written communications and it is not nice to read in code.
you can easily locate the function,
This convention is typically only used for static functions. You should know if you come across a function named like that then all you have to do is use your editor's search function to find the function in the file you're working in.
and it helps make sure no two functions will be named the same thing.
What is the problem with that?
There's no reason to make a new convention just because you think the old one is "ugly".
If the code makes me want to scratch my eyes out, I'm not likely to want to hack on it.
Rob
Robert Shearman wrote:
If the code makes me want to scratch my eyes out, I'm not likely to want to hack on it.
If you want to modify piece of code X, then code in the same style as piece of code X. Then piece of code X will all be written in the same coding style, and will be less eye-scratchy.
Use a prefix for your functions to make it easier for people that didn't write the code to find, and to avoid duplicate elf symbols for non static functions. If you don't like SHELL_ then use shell_.
There's already a mismash of coding styles in Wine, and the last thing we need is more.
Mike
-
Joris Huizer -
Mike Hearn -
Mike McCormack -
Robert Shearman