November 2012 - Wine-Devel - list.winehq.org

Re: Explicit fall-through in switch statements?

by Andrew Talbot

Perhaps I should add that the list is of caseS/defaultS that may be fallen through to, rather than out from. Unfortunately,because I produced it in a hurry, it does contain the odd copy-and-paste error for case names (e.g., the case for dmusic/collection.c line 409 should be default:, not case 8:, and that for dlls/oledb32/convert.c line 873 should be case DBTYPE_NUMERIC: not case VT_BSTR:), but I think the line numbers are more trustworthy. -- Andy.

Nov. 27, 2012

Re: ntdll/tests: Mark win8 behaviour as broken

by Dmitry Timoshkov

André Hentschel <nerv(a)dawncrow.de> wrote: > diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c > index c670731..f66d858 100644 > --- a/dlls/ntdll/tests/info.c > +++ b/dlls/ntdll/tests/info.c > @@ -1336,7 +1336,8 @@ static void test_mapprotection(void) > status = pNtQueryVirtualMemory( GetCurrentProcess(), addr, MemoryBasicInformation, &info, sizeof(info), &retlen ); > ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08x\n", status); > ok( retlen == sizeof(info), "Expected STATUS_SUCCESS, got %08x\n", status); > - ok(info.Protect == PAGE_READWRITE, "addr.Protect is not PAGE_READWRITE, but 0x%x\n", info.Protect); > + ok(info.Protect == PAGE_READWRITE || broken(info.Protect == PAGE_NOCACHE | PAGE_READWRITE /* win8 */), > + "addr.Protect is not PAGE_READWRITE, but 0x%x\n", info.Protect); This is not broken. You should either mask PAGE_NOCACHE out before the comparison, or accept both values. Same applies to other your patches. -- Dmitry.

Nov. 27, 2012

Re: Explicit fall-through in switch statements?

by Andrew Talbot

Vincent Povirk wrote: > It might be useful to post a listing of the files where unmarked > fall-throughs (falls-through?) appear, and I could see if any of them > are on my turf. Here is a rough-and-ready list of where they are. dlls/msvcp100/exception.c (line 498) case EXCEPTION: dlls/msvcp100/exception.c (line 503) case EXCEPTION_BAD_ALLOC: dlls/msvcp100/exception.c (line 508) case EXCEPTION_LOGIC_ERROR: dlls/msvcp100/exception.c (line 513) case EXCEPTION_LENGTH_ERROR: dlls/msvcp100/exception.c (line 518) case EXCEPTION_OUT_OF_RANGE: dlls/msvcp100/exception.c (line 523) case EXCEPTION_INVALID_ARGUMENT: dlls/msvcp100/exception.c (line 528) case EXCEPTION_RUNTIME_ERROR: dlls/msvcp100/exception.c (line 533) case EXCEPTION_FAILURE: dlls/comctl32/listview.c (line 5875) default: dlls/comctl32/monthcal.c (line 2059) default: dlls/comctl32/tab.c (line 3465) case WM_SETFOCUS: dlls/comctl32/toolbar.c (line 2307) case DL_CANCELDRAG: dlls/comctl32/treeview.c (line 4113) case TVHT_ONITEMLABEL: dlls/d3d10core/view.c (line 136) case D3D10_RESOURCE_DIMENSION_BUFFER: dlls/d3d10core/view.c (line 375) case D3D10_RESOURCE_DIMENSION_BUFFER: dlls/ddraw/utils.c (line 450) default: dlls/ddraw/utils.c (line 464) case 8: dlls/dmusic/collection.c (line 409) case 8: dlls/dplayx/dplay.c (line 708) case DPMSGCMD_FORWARDADDPLAYER: dlls/gdi32/bidi.c (line 515) default: dlls/gdi32/painting.c (line 161) default: dlls/gdiplus/graphics.c (line 962) default: dlls/gdiplus/graphicspath.c (line 1756) default: dlls/gphoto2.ds/gphoto2_main.c (line 1023) default: dlls/inetcomm/pop3transport.c (line 129) default: dlls/inetcomm/pop3transport.c (line 154) default: dlls/inetcomm/pop3transport.c (line 200) default: dlls/jscript/dispex.c (line 457) case PROP_PROTREF: dlls/jscript/regexp.c (line 1680) case PROP_PROTREF: dlls/jscript/regexp.c (line 1941) case PROP_PROTREF: dlls/kernel32/volume.c (line 786) case FS_FAT32: dlls/mshtml/htmlevent.c (line 1093) case FS_FAT32: dlls/mshtml/script.c (line 211) case FS_FAT32: dlls/mshtml/script.c (line 216) case FS_FAT32: dlls/msvcp90/exception.c (line 498) case FS_FAT32: dlls/msvcp90/exception.c (line 503) case FS_FAT32: dlls/msvcp90/exception.c (line 508) case FS_FAT32: dlls/msvcp90/exception.c (line 513) case FS_FAT32: dlls/msvcp90/exception.c (line 518) case FS_FAT32: dlls/msvcp90/exception.c (line 523) case FS_FAT32: dlls/msvcp90/exception.c (line 528) case FS_FAT32: dlls/msvcp90/exception.c (line 533) case FS_FAT32: dlls/msvcrtd/debug.c (line 61) case FS_FAT32: dlls/netapi32/access.c (line 950) case FS_FAT32: dlls/ntdll/reg.c (line 943) case FS_FAT32: dlls/oleaut32/variant.c (line 4424) case VT_NULL: dlls/oleaut32/variant.c (line 5512) case VT_NULL: dlls/oleaut32/variant.c (line 5519) case VT_NULL: dlls/oleaut32/variant.c (line 5533) case VT_NULL: dlls/oleaut32/variant.c (line 5542) case VT_BSTR: dlls/oledb32/convert.c (line 873) case VT_BSTR: dlls/oledlg/insobjdlg.c (line 199) case IDC_OBJTYPELIST: dlls/rsaenh/rsaenh.c (line 3780) case ERROR_SUCCESS: dlls/sane.ds/ui.c (line 1013) case WM_COMMAND: dlls/serialui/confdlg.c (line 405) default: dlls/setupapi/queue.c (line 243) case SPFILENOTIFY_STARTQUEUE: dlls/shell32/shelldispatch.c (line 709) default: dlls/shell32/shellole.c (line 696) case OPEN_ALWAYS: dlls/shell32/shellole.c (line 702) case OPEN_EXISTING: dlls/shell32/shlmenu.c (line 1226) default: dlls/shlwapi/reg.c (line 730) default: dlls/shlwapi/reg.c (line 790) default: dlls/urlmon/axinstall.c (line 258) case WM_TIMER: dlls/urlmon/internet.c (line 789) default: dlls/user32/edit.c (line 3163) case '\n': dlls/user32/mdi.c (line 1329) case SC_SIZE: dlls/user32/spy.c (line 2463) case WM_WINDOWPOSCHANGING: dlls/user32/spy.c (line 2473) case WM_STYLECHANGING: dlls/user32/spy.c (line 2523) default: dlls/usp10/bidi.c (line 773) default: dlls/usp10/breaking.c (line 131) case b_LF: dlls/wined3d/glsl_shader.c (line 467) case HEAP_NODE_TRAVERSE_RIGHT: dlls/wined3d/glsl_shader.c (line 483) case HEAP_NODE_POP: dlls/wined3d/glsl_shader.c (line 541) case HEAP_NODE_TRAVERSE_RIGHT: dlls/wined3d/glsl_shader.c (line 556) case HEAP_NODE_POP: dlls/wined3d/glsl_shader.c (line 5129) case WINED3D_TOP_BUMPENVMAP: dlls/wined3d/glsl_shader.c (line 5131) case WINED3D_TOP_BLEND_TEXTURE_ALPHA: dlls/wined3d/nvidia_texture_shader.c (line 466) default: dlls/wined3d/state.c (line 2632) default: dlls/wined3d/state.c (line 3093) default: dlls/wined3d/surface.c (line 1183) case WINED3D_TEXF_NONE: dlls/wined3d/surface.c (line 3022) case WINED3D_CONTAINER_NONE: dlls/wined3d/surface.c (line 3050) case WINED3D_CONTAINER_NONE: dlls/winex11.drv/palette.c (line 149) case GrayScale: dlls/winex11.drv/palette.c (line 178) case StaticColor: dlls/winex11.drv/xrender.c (line 1115) case AA_None: dlls/wininet/internet.c (line 3485) case INTERNET_SCHEME_GOPHER: dlls/msvcp60/exception.c (line 613) case EXCEPTION: dlls/msvcp60/exception.c (line 618) case EXCEPTION_BAD_ALLOC: dlls/msvcp60/exception.c (line 623) case EXCEPTION_LOGIC_ERROR: dlls/msvcp60/exception.c (line 628) case EXCEPTION_LENGTH_ERROR: dlls/msvcp60/exception.c (line 633) case EXCEPTION_OUT_OF_RANGE: dlls/msvcp60/exception.c (line 638) case EXCEPTION_INVALID_ARGUMENT: dlls/msvcp60/exception.c (line 643) case EXCEPTION_RUNTIME_ERROR: dlls/msvcp60/exception.c (line 648) case EXCEPTION_FAILURE: dlls/msvcp60/exception.c (line 653) default: dlls/msvcp71/exception.c (line 498) case EXCEPTION: dlls/msvcp71/exception.c (line 503) case EXCEPTION_BAD_ALLOC: dlls/msvcp71/exception.c (line 508) case EXCEPTION_LOGIC_ERROR: dlls/msvcp71/exception.c (line 513) case EXCEPTION_LENGTH_ERROR: dlls/msvcp71/exception.c (line 518) case EXCEPTION_OUT_OF_RANGE: dlls/msvcp71/exception.c (line 523) case EXCEPTION_INVALID_ARGUMENT: dlls/msvcp71/exception.c (line 528) case EXCEPTION_RUNTIME_ERROR: dlls/msvcp71/exception.c (line 533) case EXCEPTION_FAILURE: -- Andy.

Nov. 26, 2012

Re: msvcp60: Remove superfluous semicolons

by Andrew Talbot

Jacek Caban wrote: > It's probably better to change the macro to require the semicolon. > > Jacek The reason I did it that way was because there are two variants of the DEFINE_CXX_DATA macro, surrounded by an #ifndef construct: one comprising three struct declarations, all ending in semicolons, the other comprised three struct declarations (ending in semicolons) and one function definition (ending in a curly brace). I suppose I could move the function above the structs, but that would require forward declarations. -- Andy.

Nov. 26, 2012

Re: Explicit fall-through in switch statements?

by Vincent Povirk

It might be useful to post a listing of the files where unmarked fall-throughs (falls-through?) appear, and I could see if any of them are on my turf.

Nov. 26, 2012

Re: msvcp60: Remove superfluous semicolons

by Jacek Caban

On 11/26/12 8:42 PM, Andrew Talbot wrote: > Changelog: > msvcp60: Remove superfluous semicolons. > > diff --git a/dlls/msvcp60/exception.c b/dlls/msvcp60/exception.c > index 761075e..b8c6921 100644 > --- a/dlls/msvcp60/exception.c > +++ b/dlls/msvcp60/exception.c > @@ -137,7 +137,7 @@ void * __thiscall MSVCP_exception_vector_dtor(exception *this, unsigned int flag > } > > DEFINE_RTTI_DATA0(exception, 0, ".?AVexception(a)std@@"); > -DEFINE_CXX_DATA0(exception, MSVCP_exception_dtor); > +DEFINE_CXX_DATA0(exception, MSVCP_exception_dtor) It's probably better to change the macro to require the semicolon. Jacek

Nov. 26, 2012

Re: Explicit fall-through in switch statements?

by Andrew Talbot

Frédéric Delanoy wrote: > For every wine version, static checkers (like coverity) detect cases > where a switch case automatically falls-through to the next case. > > Shouldn't be there a rule that such cases are always marked with a > "fall-through comment"? > With the possible exception of case with no statement? like 'case FOO: > case BAR: do_something();...'? > > This would probably help IMHO, especially when people refactor old code. > > Frédéric I reckon there are currently about 105 unmarked fall-throughs in the dlls. Unless people consider it offensive, one option might be to make careful use of git blame to find out who wrote the particular cases and ask them whether their fall-throughs were deliberate, marking up or fixing with breakS as appropriate. -- Andy.

Nov. 26, 2012

Re: [PATCH 1/5] advapi: Implement SetNamedSecurityInfoW on top of SetSecurityInfo (try 2).

by Alexandre Julliard

"Erich E. Hoover" <ehoover(a)mymail.mines.edu> writes: > This particular patch implements SetNamedSecurityInfoW by layering it > on top of SetSecurityInfo, which permits the creation of tests for > part 2 (GetNamedSecurityInfoW). This version has been updated to fix > a bad return value and includes a test indicating the correct value > (ERROR_FILE_NOT_FOUND). That doesn't mean you can hardcode that specific error. -- Alexandre Julliard julliard(a)winehq.org

Nov. 26, 2012

Re: [1/2] ole32: Add helper for string table memory freeing

by Frédéric Delanoy

On Mon, Nov 26, 2012 at 8:54 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On 11/26/2012 01:01, Frédéric Delanoy wrote: >> >> --- >> dlls/ole32/filemoniker.c | 44 >> ++++++++++++++++++-------------------------- >> 1 file changed, 18 insertions(+), 26 deletions(-) >> >> diff --git a/dlls/ole32/filemoniker.c b/dlls/ole32/filemoniker.c >> index b3a5cc2..688f6b8 100644 >> --- a/dlls/ole32/filemoniker.c >> +++ b/dlls/ole32/filemoniker.c >> @@ -657,6 +657,16 @@ FileMonikerImpl_Reduce(IMoniker* iface, IBindCtx* >> pbc, DWORD dwReduceHowFar, >> return MK_S_REDUCED_TO_SELF; >> } >> +static void >> +FileMonikerImpl_FreeStringTable(LPOLESTR* stringTable) >> +{ >> + int i; >> + >> + for (i=0; stringTable[i]!=NULL; i++) >> + CoTaskMemFree(stringTable[i]); >> + CoTaskMemFree(stringTable); >> +} >> + > > Could you please adjust a naming for that to something like > free_stringtable() cause it doesn't really access file moniker in this > helper. OK >Also stringTable[i]!=NULL could be shortened to stringTable[i]. I know but it's mostly a matter of style. I prefer the '!= NULL' notation when accessing it "array-style", and any sane compiler would optimize it anyway. >> CoTaskMemFree(str1); >> CoTaskMemFree(str2); >> @@ -1004,17 +1010,9 @@ FileMonikerImpl_CommonPrefixWith(IMoniker* >> iface,IMoniker* pmkOther,IMoniker** p >> { >> for(i=0;i<sameIdx;i++) >> strcatW(commonPath,stringTable1[i]); >> - >> - for(i=0;i<nb1;i++) >> - CoTaskMemFree(stringTable1[i]); >> - >> - CoTaskMemFree(stringTable1); >> - >> - for(i=0;i<nb2;i++) >> - CoTaskMemFree(stringTable2[i]); >> - >> - CoTaskMemFree(stringTable2); >> - >> + >> + FileMonikerImpl_FreeStringTable(stringTable1); >> + FileMonikerImpl_FreeStringTable(stringTable2); > > You sure it's an equivalent change? I mean regarding index boundaries above. It's equivalent. > Also while you're at it FileMonikerImpl_DecomposePath() needs to be > properly fixed - it returns HRESULT, not it. And surrounding code relies on > FAILED() mask to get this index boundary from return value, this is really > broken. Yes, but that probably warrants a patch of its own, to keep this one as self-contained/atomic as possible. I'll look into that Frédéric

Nov. 26, 2012

Re: [1/2] ole32: Add helper for string table memory freeing

by Nikolay Sivov

On 11/26/2012 01:01, Frédéric Delanoy wrote: > --- > dlls/ole32/filemoniker.c | 44 ++++++++++++++++++-------------------------- > 1 file changed, 18 insertions(+), 26 deletions(-) > > diff --git a/dlls/ole32/filemoniker.c b/dlls/ole32/filemoniker.c > index b3a5cc2..688f6b8 100644 > --- a/dlls/ole32/filemoniker.c > +++ b/dlls/ole32/filemoniker.c > @@ -657,6 +657,16 @@ FileMonikerImpl_Reduce(IMoniker* iface, IBindCtx* pbc, DWORD dwReduceHowFar, > return MK_S_REDUCED_TO_SELF; > } > > +static void > +FileMonikerImpl_FreeStringTable(LPOLESTR* stringTable) > +{ > + int i; > + > + for (i=0; stringTable[i]!=NULL; i++) > + CoTaskMemFree(stringTable[i]); > + CoTaskMemFree(stringTable); > +} > + Could you please adjust a naming for that to something like free_stringtable() cause it doesn't really access file moniker in this helper. Also stringTable[i]!=NULL could be shortened to stringTable[i]. > > > CoTaskMemFree(str1); > CoTaskMemFree(str2); > @@ -1004,17 +1010,9 @@ FileMonikerImpl_CommonPrefixWith(IMoniker* iface,IMoniker* pmkOther,IMoniker** p > { > for(i=0;i<sameIdx;i++) > strcatW(commonPath,stringTable1[i]); > - > - for(i=0;i<nb1;i++) > - CoTaskMemFree(stringTable1[i]); > - > - CoTaskMemFree(stringTable1); > - > - for(i=0;i<nb2;i++) > - CoTaskMemFree(stringTable2[i]); > - > - CoTaskMemFree(stringTable2); > - > + > + FileMonikerImpl_FreeStringTable(stringTable1); > + FileMonikerImpl_FreeStringTable(stringTable2); You sure it's an equivalent change? I mean regarding index boundaries above. Also while you're at it FileMonikerImpl_DecomposePath() needs to be properly fixed - it returns HRESULT, not it. And surrounding code relies on FAILED() mask to get this index boundary from return value, this is really broken.

Nov. 26, 2012