Coding Hints

From Wine-Wiki.org
Jump to: navigation, search

Contents

Working with Data

This is not official documentation nor is this a guide. It is a documentation of comments from the mailing list. Opinions and standards change for any project and some of this may become out of date. If you spot a mistake, feel free to correct it, but note the date so others can identify how recent it was.

Data types: Strings, boolean etc

Parameter Naming

One patcher was told by A. Julliard not to change the name of a parameter [wine devel 09 jan]: 'flags' is a perfectly good name.

The developer noted: It was just to reflect more the MSDN. Maybe it's not that usefull.

A. Julliard: No it's not, MSDN uses the stupid Hungarian notation everywhere, and we should stay as far away as possible from that monstrosity. 'flags' is a much better name than 'dwFlags'.

Typecasting

diff --git a/dlls/avifil32/avifile.c b/dlls/avifil32/avifile.c index f989c79..e7b22b1 100644
--- a/dlls/avifil32/avifile.c
+++ b/dlls/avifil32/avifile.c
@@ -835,7 +835,7 @@ static LONG WINAPI IAVIStream_fnFindSamp
 	ULONG n;
 
 	for (n = 0; n < This->sInfo.dwFormatChangeCount; n++)
-	  if (This->idxFmtChanges[n].ckid >= pos) {
+	  if (This->idxFmtChanges[n].ckid >= (ULONG)pos) {

A. Julliard [Feb 23 2007]: Please don't add typecasts all over the place. If things can be fixed by changing variable types then that's fine, but otherwise we can just as well live with the warnings.[wine devel list]


Portability

A developer [Jun 06] asked if his patch was rejected because of use of L"" syntax?

J. Hawkins: Probably, L"" is not portable, and you have to use:

static const WHCAR string[] = {'s','t','r','i','n','g',0};

M. McCormack: wchar_t is an int on some versions of gcc. It can be made a short using -fshort-wchar, but only later versions of gcc (3.0+) support that flag.

M. Meissner: In Linux, L"" makes 4 byte character strings (if not using explicit -fshort-wchar). Also, if the user then calls wc* functions from glibc, they will use 4 byte characters. It is all to avoid confusion. wine archive

> +static
> +void serialize_dword(DWORD value,BYTE ** ptr)
> +{
> +    /*TRACE("called\n");*/
> +
> +    *((DWORD*)*ptr)=value;
> +    *ptr+=sizeof(DWORD);

A. Julliard: This isn't safe for CPUs that don't allow unaligned accesses, you have to use memcpy instead.


+     for (i = 0; i < ifc.ifc_len / sizeof(struct ifreq); i++)

J. Lang:[Dec 05] I was concerned about this. This isn't portable. In some systems, struct if_req occupies fixed space in ifc, but in others, the size depends on the amount of data in each struct if_req. You need to use ifreq_len.wine archive See e.g.: http://source.winehq.org/source/dlls/iphlpapi/ifenum.c#L201

* [n][0] == pointer to registry subkey name (WCHAR) 
* [n][1] == type of registry entry
* [n][2] == pointer to buffer to store setting 
* [n][3] == size of data returned
* [n][4] == default value to use if subkey was not found 
* [n][5] == registry path to open (ANSI, not including root (HKEY_CURRENT_USER,etc)) */
DWORD settings_data[15][6] = {
    { (DWORD)&heightW, REG_DWORD, (DWORD)&Globals.lfFont.lfHeight,
      sizeof(REG_DWORD), -10, (DWORD)&FONT_SETTINGS_REG_PATH },

R. Shearman [Jun 06]: You should define a proper structure to put this data in rather than casting the values to DWORD (which won't work in 64-bit mode). wine archive


proc

For my curiosity, why can't we use /proc for card detection?

S. Dosinger: Because it isn't portable. Wine runs not only on Linux, but also on Solaris, Mac OS X, *BSD, ...

Numbers

Raphael [July 05]: its better to use unsigned for indexes and counts Wine Archive



+  if(PFCI_INT(hfci)->fNextCab==TRUE) {

Dimi Paun [Jun 05]: It's better to avoid explicit comparison with TRUE. What if fNextCab is 10, not 1? It's still true, but it will fail the above test. Just do:Wine Archive

+ if(PFCI_INT(hfci)->fNextCab) {


Strings

+      RegSetValueExA(hkey, "HwProfileGuid", 0, REG_SZ, (const BYTE*) debugstr_guid(Uuid), strlen(debugstr_guid(Uuid)));

R. Shearman: You shouldn't be using a debugging command for formating a GUID to a string, as the function could change its output later on and break this function. wine archive


if (CharPrevA(szFilename, szFilename + iLen) != "\\")
{

M. Hern: [This] should be

if (CharPrevA(szFilename, szFilename + iLen) != '\\')
{

Note the single quotes, which makes this a character instead of a string (so it can be compared using !=). [...] You need to use when strcatting as well, if you are actually dealing with backslashes.wine archive


strcpy
+   strncpy(lpszName, devname, cbName);

J. Lang You won't be able to use strncpy anymore, see Alexandre's commit:http://www.winehq.org/hypermail/wine-cvs/2005/04/0275.html


+      strcpy(name, caps.card);

J. Lang: Please don't use strcpy. Even though caps.card and name happen to be declared of the same size, and caps.card is very likely null-terminated, it takes a bit of searching around to make sure that's the case. It's easier to verify it's correct if you use strncpyA or memcpy instead. Since you NULL-terminate in the next line, I'd suggest memcpy.


strcmpW
+                if(!strcmpW(system_font_link->font_name,font_link->font_name))
+                {
+                    duplicate = TRUE;
+                    break;
+                }
+            }

[Apr 09] D. Timoshkov Font name comparison should be case insensitive. If there are other places that do that they should be fixed.

strcasecmp

A developer wrote [Feb 08 wine devel] If a Wine DLL uses strcasecmp() then it's a bug. It should use an appropriate kernel32 API instead.

F. Gouget: The situation is not all that bad. I count only 158 references to strcasecmp(). So reviewing and fixing those that are wrong seems quite feasible and would make a good janitorial task. The rules are as follows:

strlen
+    *lpdwUrlLength += strlen("://");

One programmer felt: You should use sizeof instead to avoid the call to strlen. M. Meissner wrote: Please just keep it as is. The compiler knows strings constants and strlen() and will replace it by a constant on compile time. A. Mohr: Indeed, doing a small test with standard optimization confirms it, but I really didn't expect this to be the case.


printf
+      sprintf(version, "%s v%u.%u.%u",

J. Lang: It's also not obvious how large version is. I'd recommend passing in the >length of the buffer and using snprintf instead. You might as well pass in the length of name, too.



   if( flags->IntegerSize == INTEGERSIZE_LONGLONG )
 +    {
 +        sprintf(p, "l");
 +        p++;
 +    }

A. Julliard [Jun 05]: This assumes that the system printf supports %ll formats, that's not portable Wine Archive


 +    if (NULL == lpddsd) {
 +      DPRINTF("(null)\n");
      } else {

One programmer asked: Please don't use DPFINTF Wine Archive However another developer disagreed, saying they have made use of it and it has been accepted for wine.


+    {
+        helper->pipe_in = fdopen(pipe_in[0], "r");
+        close(pipe_in[1]);
+        helper->pipe_out = fdopen(pipe_out[1], "w");
+        close(pipe_out[0]);
+    }

A. Julliard: You should avoid using stdio functions like fdopen, fgets, etc. They are not guaranteed to be thread-safe because of the way we handle libc threads. It's better to use normal read()/write() instead. Wine Archive

GUID
+    lpBracedGuidString = (LPWSTR) HeapAlloc(GetProcessHeap(), 0, 
+    (1 + strlenW(lpGuidString) + 1 + 1) * sizeof(WCHAR)); 
+    lpBracedGuidString[0] = (WCHAR) '{'; 
+    memcpy(&lpBracedGuidString[1], lpGuidString, + strlenW(lpGuidString) * sizeof(WCHAR)); 
+    lpBracedGuidString[1 + strlenW(lpGuidString)] = (WCHAR) '}'; 
+    lpBracedGuidString[1 + strlenW(lpGuidString) + 1] = (WCHAR) 0; 
+    RpcStringFreeW(&lpGuidString);

V. Margolen: [Sept 06] All that looks messy and way too many calls to strlenW for no reason. Length of GUID string is known and hard-coded to 36. wine archive

lpBracedGuidString = HeapAlloc(GetProcessHeap(), 0, (36 +1+2) *
sizeof(WCHAR)); memcpy(lpBracedGuidString + 1, lpGuidString, 36 *
sizeof(WCHAR)); lpBracedGuidString[ 0] = '{';
lpBracedGuidString[37] = '}';
lpBracedGuidString[38] = 0;
RpcStringFreeW(&lpGuidString);
String Array
+  char useragent[] = {'W','i','n','i','n','e','t',' ','T','e','s','t',0 };

A. Mohr [Nov 05]: [This will work better] wine archive

static const char useragent[] = "Wininet Test";



Noticed in dlls/msi Mike McCormack said: Use "static const" rather than "const static" as gcc -W complains. Patch: http://cvs.winehq.org/patch.py?id=20960


+    MultiByteToWideChar(CP_ACP, 0, pPerUser->szVersion, 32, perUserW.szVersion, 32);
+    MultiByteToWideChar(CP_ACP, 0, pPerUser->szCompID, 128, perUserW.szCompID, 128);

A. Julliard: The source lengths are wrong, these are null-terminated strings. Also please use sizeof() instead of hardcoding the constants. A developer asked Would MultiByteToWideChar(CP_ACP, 0, pPerUser->szGUID, -1, perUserW.szGUID, sizeof(perUserW.szGUID)) be a better solution?

M. McCormack: The counts for both A and W are in characters, and sizeof() gives bytes, so you need to divide the sizeof a WCHAR array by sizeof(WCHAR) to get the number of characters in the array. wine archive

Windows Paths

D. Riekenberg feb09: Hardcoding "c:\windows" is broken by design, since that path can be changed during installation of Windows and even the default path is different in some Windows versions. ("c:\WinNT" on w2k as example)

VARIANT

A developer asked Nov 08: My question is, should my next version of the patch list the VARIANT as 4 longs, or is there some better way to specify this that I'm not aware of? If not, is there a comment syntax for the spec file where I can explain why there are 4 long params where there should be one VARIANT?

M. Messiener: Hmm, use as much "long" specifiers to fit the VARIANT struct (number of bytes/4). I think VARIANT has 16 byte, so use "long long long long". [...] Currently no other way.

Internationalisation

+ if (result == -1) {     /* text not found. */ 
+     MessageBoxA(Globals.hEdit, "Cannot find text.", NULL, MB_OK | MB_ICONINFORMATION); 
+     } else {

V. Margolen [Jun 06]: You should put all the text into resource files. wine archive


+LANGID WINAPI SetThreadUILanguage(WORD wReserved)
+{
+  FIXME("SetThreadUILanguage(0x%04x), semi-stub! (defaulting to english)\n", wReserved);
+ return MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT);
+}

D. Timoshkov [July 05]: Why are you returning english here? Wine should display UI according to current user locale, so GetUserDefaultUILanguage() would be more appropriate here. Wine Archive


+static const char *errorstring[] = {
+    "Success",
...
+static const WCHAR error00[] = { 'S','u','c','c','e','s','s',0 };

D. Timoshkov: I'd suggest to move all strings into a stringtable and load them by LoadStringA/W. That way you get internationalization for free.

H. Liedekker: according MSDN: 'The return value is a static pointer to the character string. Do not free this string'. How would you handle this using LoadString?

D. Timoshkov: Something like this:

PCHAR ldap_err2stringA( ULONG err )
{
static char buf[256];
if (!LoadStringA(wldap_hinst, err, buf, 256))
LoadStringA(wldap_hinst, IDS_DEFAULT_ERROR, buf, 256);
return buf;
}
PWCHAR ldap_err2stringW( ULONG err )
{
static WCHAR buf[256];
if (!LoadStringW(wldap_hinst, err, buf, 256))
LoadStringW(wldap_hinst, IDS_DEFAULT_ERROR, buf, 256);
return buf;
}

Sending Patches and Foreign Languages

A programmer/translator asked about patches when there is more than one language for a locale: the README.no file in documentation/ should be renamed README.nb. There are two variants of Norwegian: [..] (nb) and Nynorsk (nn).

A. Juliard [sept05]: The idea is the default language gets the main name, then you can add a suffix, for instance we have README.pt and README.pt_br. Wine Archive

M. Zalewski [Oct 06] commented on SUBLANG_NEUTRAL and SUBLANG_DEFAULT: [...]there is only one sublang. SUBLANG_NEUTRAL is a fallback that is used if no translation is available in the user's selected sublang. In can be used for locales that have many sublangs (e.g. French have French (France), French (Belgian), French (Canada), French (Swiss) etc) but the language is the same so it's enough to make one translation. wine archive

He also created an official Wine wiki page with relevant technical information:


Adding a string resource

A developer [Nov 06] (edited): I want to add a new string resource to the winhelp program. I've modified only the English-localized .rc file. My locale is Ru. When I try to load this string, LoadString() function returns 0 and GetLastError() returns 0 too. I expect, that English string will be loaded, but no string is loaded.

When I explicitly set locale:

 LC_ALL=C winhelp

then the string is loaded OK (nothing else expected).

So, the question: is this behaviour of resource loader correct? Should it fallback to LANG_ENGLISH SUBLANG_DEFAULT if localized string not found and LANG_NEUTRAL/SUBLANG_NEUTRAL is requested (as LoadString does)?

A. Julliard: Strings are stored by blocks of 16, so if others strings in the same block are defined for Russian the resource loader won't fall back to English even if individual strings are missing. wine archive

He then queried: So, what is the right way to add a new string?

A. Julliard: If you are adding multiple strings then the easiest is to start a new block. Otherwise if it's a single string, or it logically belongs to an existing block, then yes you have to add it everywhere if you want a fallback.


Conclusion: a new string should be added untranslated to every locale-specific file: Bg.rc ,Cs.rc, Da.rc... Otherwise the string will not be loaded in some locales.

Unicode vs Ascii Characters

A Programmer noted: There are several occurences in my patches, where I would have prefered to use the ascii APIs (especially in RegQueryValue and the like). Would that be ok?

Dimi Paun: I think it would be better to always use the W APIs. This way we can have tools to warn if we have W->A transitions. We lose a little in readability (a special grep tool anyone?), but we gain a simple rule to follow and enforce, which could be used by various tools, like winapi_check.

F. Nawothnig: I'm sure this has already been proposed (and rejected :) but I'll ask nevertheless: Why not use L"" everywhere and add a small perl/awk/whatever preprocessor which translates into C89 for compilers which don't support wide string literals?


Further Reading


D. Riekenberg [Oct 05]: Defining a Unicode-String with L"text" does not work with gcc on unix. Please use the same Style as your Patch did for wProperties in "shlexec.c" (Array of WCHAR).

D. Riekenberg [Oct 05]: TCHAR is not used in wine. If there is a UNICODE and a ANSI implementation in Windows, implement the UNICODE-Version and let the ANSI-Version call the UNICODE-Version. Wine Archive


One developer wrote [sept 09 wine devel] but I think you want to make [new programs in general] explicitly unicode from the start. M. Stefanuic agreed: Converting a program to unicode is not hard work per se but time consuming and boring.

Conversion between wide characters and multi byte characters

Anon [July 05]: I just noticed that your unixfs code appears to be using CP_ACP for all conversions between wide characters and multi byte characters. This is OK for multi byte display names, but for file names being used in or received from the UNIX file system, CP_UNIXCP is probably more appropriate. See wcstoumbs in ntdll, which uses unix_cptable, set by kernel32, which uses the same table for CP_UNIXCP Wine Archive


Further reading

Pointers

R. Shearman:

+  EnumMonikerImpl* This=0;

Using NULL instead of 0 tends to make it clearer that you are using a pointer.

UINT32

+((PIMAGE_SECTION_HEADER)((LPBYTE)&((PIMAGE_NT_HEADERS)(UINT32)(ntheader))->OptionalHeader + \ 
+                           ((PIMAGE_NT_HEADERS)(UINT32)(ntheader))->FileHeader.SizeOfOptionalHeader))

D. Timoshkov [Oct 06]: UINT32 is not a 64-bit safe type. Please use ULONG_PTR instead.

E. Pouch: nothing against ULONG_PTR, [...] my proposal only has one ugly ULONG_PTR cast,[...] this would be better written as wine archive:

((PIMAGE_SECTION_HEADER)(DWORD_PTR)((LPCBYTE)&((const IMAGE_NT_HEADERS*)(ntheader))->OptionalHeader + \

+ ((const IMAGE_NT_HEADERS*)(ntheader))->FileHeader.SizeOfOptionalHeader))

Union

+    union {
+        WINE_WAVEOUT    out;
+        WINE_WAVEIN     in;
+    };

A. Julliard: You need to give a name to the union, anonymous unions are not portable. Though considering how few fields remain in the in/out structure you might just as well merge them into the common structure and use a single structure for everything.

     struct dce *dce;            /* DCE for CS_OWNDC or CS_CLASSDC windows */
     HBITMAP     hWMIconBitmap;
     HBITMAP     hWMIconMask;
+    union {
+        struct xembed_list *list;
+        ULONG_PTR cnt;
+    } xembed;
};

Dimi Paun: Wine Archive Do we really need the union? We're not that hard pressed for memory. What about just

@@ -649,6 +662,10 @@ struct x11drv_win_data
struct dce *dce; /* DCE for CS_OWNDC or CS_CLASSDC windows */
HBITMAP hWMIconBitmap;
HBITMAP hWMIconMask;
+ struct xembed_list *list;
+ ULONG_PTR cnt;
};



Allocating Memory

A programmer asked why not free the allocated Memory as in my Patch? [Was it] Just to Save some Bytes of Code or is This is done automatically, when the Program exit's

A. Julliard: Yes, there's no point in explicitly freeing memory on process exit, it's done automatically. Wine Archive



+struct mem_area
+{
+    struct list   entry;       /* Entry in global mem area list */
+    const void   *base;        /* Base address */
+    UINT          size;        /* Size in bytes */
+    void         *data;        /* Data associated with this area */
+};

D. Timoshkov: It's better to change the size type to SIZE_T from UINT to allow 64-bit size memory allocations on (future) Win64.



+ unsigned char buffer[SECUR32_MAX_BUF_LEN+6000];
+ BYTE bin[SECUR32_MAX_BUF_LEN];

A. Julliard: You shouldn't allocate fixed size buffers on the stack, especially not huge ones like that. You should compute the necessary size and allocate a properly sized buffer from the heap. Wine Archive


malloc()

A programmer asked Why is malloc() evil even if it is only used internally and its memory is freed before returning the results?

F. Gouget: I think this is because it introduces a dependency on msvcrt.dll on the Windows and ReactOS platforms. So it's better to stick to 'pure' Win32 APIs as much as possible. Wine Archive

alloca()

> +#define TMP_ALLOC(s) alloca(s)

F. Gouget: Doesn't HeapAlloc work on any 'Win32' platform?

The programmer replied: I wanted to avoid calling heap allocation functions. Using alloca() is slightly lighter.

Dimi Paun: Please don't do that. It uglifies the code, and makes it less reliable. alloca() is lighter but it's on the stack, and I really doubt that you can measure the difference anyway May05 Wine Link.


realloc

When compiling Wine for Solaris M. Greenbank reported an error 'Virtual Memory has been exhausted'

E. Frias : It is probably this line:

imp->exports = xrealloc( imp->exports, imp->nb_exports *  sizeof(*imp->exports) );

The behavior of realloc(ptr, 0) is undefined, but linux lets you do it so it wasn't caught. Solaris isn't so forgiving. Neither is HPUX, for that matter.

According to the spec (found this online, I think it's quoting C99:

"If the size of the space requested is zero, the behavior is implementation-defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object."

You should change it to:

 imp->exports = xrealloc( imp->exports, imp->nb_exports ? (imp->nb_exports * sizeof(*imp->exports)) : 1 );

or something similar to get it to work. Wine Archive Link

HeapAlloc

 +        if (!table)
         {
 -            req->atom = atom;
 -            req->table = table;
 -            wine_server_set_reply( req, full_name, sizeof(full_name) );
 -            if (!wine_server_call_err( req ))
 +            ATOM_BASIC_INFORMATION *abi;
 +
 +            size = sizeof(*abi) + MAX_ATOM_LEN * sizeof(WCHAR);
 +            abi = HeapAlloc(GetProcessHeap(), 0, size);

D. Timoshkov:[Sept 05] Using HeapAlloc will considerably slowdown the function, you can safely allocate the necessary buffer on the stack. The same for GetAtomNameW. Wine Archive


HeapFree

+#define HH_Alloc(size) ((LPVOID)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size))
+
+#define HH_Free(buffer) \
+    if (buffer) \
+        HeapFree(GetProcessHeap(), 0, (HLOCAL)buffer);

D. Timoshkov [July 05]: Alot of work has been done to avoid tests for NULL before HeapFree call, please do not introduce them again. Wine Archive

+       if (pbBuffer != NULL)
+               HeapFree(GetProcessHeap(), 0, pbBuffer);

D. Riekenburg[Jul 06]: HeapFree() handles NULL; we removed similar unneeded "if" recently. wine archive

Functions

A. Julliard Jan 08 winedevel: please don't make up names for ordinal functions, use anonymous entry points ('@') until we find out the real name.

> +static void infinstall_test()

S. Huehner: please use

static void infinstall_test(void)

for the above line, as this is more correct c code and avoids the warning enabled with (-Wstrict-prototypes). wine archive


A. Julliard [July 05]: Functions that are not used in other files should be declared static, you should only export functions that really have to be used somewhere else. Wine Archive

 +(PSP_FILE_CALLBACK_A) LaunchINFSectionEx_CabExtractCallBackA, 
 +(LPVOID) inf_name);

R. Shearman [Nov 05]: Don't cast LaunchINFSectionEx_CabExtractCallBackA. That's a recipe for using a function with an incorrect calling convention and trashing the stack. [...]Just fix up the declaration of that function if it issues a warning here. wine archive


+#define CALLANYPAINTHOOK(msg, wParam, lParam) \
+    (((pda->dlga->Flags & PSD_ENABLEPAGEPAINTHOOK) && \
+    pda->dlga->lpfnPagePaintHook(hWnd,msg,wParam,lParam)) || \
+    PRINTDLG_DefaultPagePaintHook(hWnd,msg,wParam,lParam))

A. Julliard: It would be a lot cleaner to always call your DefaultPagePaintHook function and have it call the user's hook first if it's set.


A programmer noted [Aug 05]: MSXML_DllGetClassObject() is missing the WINAPI. This causes winapi_check to issue an error.

R. Shearman: That wouldn't have happened if the function had been called DllGetClassObject instead of MSXML_DllGetClassObject. Is the uniqueness of the name really more important than the compiler being able to typecheck it? Can we please not introduce any more of the Dll* functions with DLLNAME_ prefixes Wine Archive


 +    (a)->rootdir    = (r)->rootdir; \
 +    (a)->attributes = (r)->attributes; \
 +    (a)->name       = (n);

A. Julliard: [Nov 05] please pass parameters explicitly to the functions, don't hide them inside an object_attr structure wine archive

Using Const and Functions

A programmer noted: marking a couple of parameters of [a commonly called function] const (that I believe should be marked const anyways), CPU usage in that function drops measurably [1.5% performance improvements] with oprofile.[...] Just changing the parameters to const for the function in question (and none of the other functions in the file) increases the size from the original size by 64 bytes.

T. Rollo was cautious: In fact I don't know that making these const would make any difference - I'd like to see the assembly code generated by the two versions for comparison. The 1.5% appears to be within the noise.

A few of us analysed those routines to death on IRC a couple of months back and decided the real problem was that they were being called too frequently, not that there was a problem with the routines themselves. The routines could probably be made faster by rewriting them in assembly language, but bigger benefits could be gained by figuring out why there are so many calls and reducing the number of calls.

J. Garvin too was cautious with such a small gain: Has anyone checked if the performance improvement stays if only the pointers are set to const? I'd also suggest running the test multiple times and averaging.

F. Nawothnig: There is no need to make anything except the pointers const [...] In theory this would give the compiler slightly more information... but if the optimizer is unable to figure out that the parameter is never used as an lvalue by himself it sucks so badly that it probably won't do much better with that hints anyway. :-)

Constifying the pointers is fine ofcourse (but rather because it helps finding bugs than for those 1.5% performance improvements.

A. Mohr explained how it assists finding bugs: it should be a good idea to mark things in Wine const whenever possible (objdump -x helps here), since it improves reliability (erroneous random memory area writes will crash in const areas) and performance (hints for the compiler in some non-optimized compiler cases, page discarding instead of swapping). [With the difference noted in binary size] You probably need to strip the binary or omit gcc -g in order to obtain meaningful numbers, since otherwise it probably encodes the additional const in debug information tables, too, which thus increases the size of the binary. Wine Archive


Naming

+ 704 stdcall @(wstr ptr) SHELL32_704 # GUIDFromStringW

A developer asked: Out of curiosity, what's the reason for using SHELL32_704 as the name, instead of GUIDFromStringW? and was told: I just followed a pattern already there in shell32, I don't know the exact rules that apply here, to be honest.

A. Julliard: If there's a known official name you should use it, and most likely make it a -noname export.

Locations

[Ed.funny enough, We were not sure where to put this text.]

diff --git a/dlls/rpcrt4/rpcrt4.spec b/dlls/rpcrt4/rpcrt4.spec index 2c86e68..56a55f6 100644
--- a/dlls/rpcrt4/rpcrt4.spec
+++ b/dlls/rpcrt4/rpcrt4.spec
@@ -203,7 +203,7 @@
@ stub NdrGetSimpleTypeBufferSize # wxp @ stub
NdrGetSimpleTypeMemorySize # wxp @ stub NdrGetTypeFlags # wxp
-@ stub NdrGetUserMarshallInfo
+@ stdcall NdrGetUserMarshalInfo(ptr long ptr)
@ stub NdrHardStructBufferSize #(ptr ptr ptr) @ stub NdrHardStructFree
#(ptr ptr ptr) @ stub NdrHardStructMarshall #(ptr ptr ptr)
diff --git a/dlls/rpcrt4/rpcrt4_main.c b/dlls/rpcrt4/rpcrt4_main.c index 537a005..1a6b61d 100644
--- a/dlls/rpcrt4/rpcrt4_main.c
+++ b/dlls/rpcrt4/rpcrt4_main.c
@@ -1016,3 +1016,9 @@ RPC_STATUS RPC_ENTRY RpcCancelThreadEx(void* ThreadHandle, LONG Timeout)
else
return rpc_cancel_thread(target_tid);
}
+
+RPC_STATUS RPC_ENTRY NdrGetUserMarshalInfo(ULONG *flags, ULONG level, NDR_USER_MARSHAL_INFO *mi)
+{
+ FIXME("(%p, %u, %p)\n", flags, level, mi);
+ return RPC_X_INVALID_BUFFER;
+}

R. Shearman jan 09: Since this function has the prefix Ndr it should go in an ndr_*.c file in rpcrt4, not one of the others (I would suggest ndr_marshall.c next to the user marshal functions, which initialise the data structure that it operates on). It also lacking a winapi comment.

Internal Static Function Style

+static inline int PROGRESS_GetLedSize( PROGRESS_INFO *infoPtr, LONG style,

Dimi Paun [July 05]: It has become the norm lately to not have the ALLCAPS_ prefix for internal static functions, but rather have names_with_underscores to easily tell them apart from Win32 APIs. So for the above, I guess get_led_size() would be preferable.

The ALLCAPS prefix comes from a day long gone when we didn't have DLL separation. Now it just hurts the eye and it's distracting, without providing much benefit for internal functions.

I think we're safe to assume that the prefered rule is:

Functions to improve Clarity

+    if(data->xembed.cnt)

Dimi Paun:Wine Archive Maybe we can have a function to make it more explicit what we're testing for:

BOOL win_has_embedded_children(struct x11drv_win_data *data)

Goto

M. Stefanuic [wine devel feb 08]: Wine takes the right (TM) approach to goto: the non ideological one: goto is fine for handling a lot and complex cleanups needed on error paths. You always need to balance the advantages and disadvantages of goto: does the goto make the code more readable and clear? If yes use the goto if not then don't. See also Donald Knuth paper "Structured Programming with Goto Statements" http://pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pdf [...] Using goto right is an art; you need to look at the code and say that it looks good.

16bit Functions

A developer noticed [Jan 09]: The krnl386.exe.spec file points _hread() to WIN16_hread() so that this _hread16() function is unused. The naming of these implementations is a bit inconsistent but it may not be worth a patch to fix it (but if it is I'll gladly send one).

A. Julliard: The reason for the inconsistent naming is that the xxx16 functions are 16-bit implementations usable from normal Wine code, while the WIN16_xxx ones have different calling sequences and are for use by the relay code only. That's why we had both WIN16_hread and hread16. Of course at this point we shouldn't be calling 16-bit functions from Wine code, so it no longer matters.


Return Values

+    if(helper->helper_pid == 0)
+    {
+        /* We're in the child now */
+        close(0);
+        close(1);
+        
+        dup2(pipe_out[0], 0);
+        close(pipe_out[0]);
+        close(pipe_out[1]);
+
+        dup2(pipe_in[1], 1);
+        close(pipe_in[0]);
+        close(pipe_in[1]);
+
+        execvp(prog, argv);
+
+        /* Whoops, we shouldn't get here. Big badaboom.*/
+        return SEC_E_INTERNAL_ERROR;

A. Julliard: You shouldn't return from the child, you should pass the error back to the parent and then exit().Wine Archive


NT API

+    ok( rc == S_OK, "Create MailslotFile failed rc = %x %u\n", rc, GetLastError());

D. Timoskov [Feb 07]: NT APIs return STATUS_xxx error codes, S_OK is an OLE error value.

+    ok( hslot != INVALID_HANDLE_VALUE, "Handle is invalid\n");

INVALID_HANDLE_VALUE is a kernel32 thing, NT APIs don't use it. Most likely you need to test returned value against 0.

+    ok( CloseHandle(hslot), "CloseHandle failed\n");

To close the handle returned by an NT API you have you use an NT API as well. The developer qeried this: Have reread MSDN http://msdn2.microsoft.com/en-us/library/ms648410.aspx > and it seems that NtClose has been superseded by CloseHandle. So I suppose that CloseHandle is the one to use.

D. Timoshkov: The information in MSDN is misleading in this case, CloseHandle is a kernel32 API, not an NT (native) one. The developer again queried: So the correct move is to use the depreciated function? D. Timoshkov: It's not deprecated, even Microsof tries to create an impression it is.

SUCCEEDED Macro

+    return SUCCEEDED(RegSetValueExW(hkeyOpen,wszName,0,dwType,
+                                    pData.pbData,pData.cbData));

M. McCormack:The SUCCEEDED() macro is only for HRESULT values, so the above is going to succeed in alot of cases where it shouldn't. Wine Archive Link Italic text

E_NOTIMPL Macro

> +   return E_FAIL;

M. McCormack: return E_NOTIMPL for stuff that isn't implemented.


+static HRESULT WINAPI KSP_Set(IKsPropertySet * iface, REFGUID guidPropSet, DWORD dwPropID, LPVOID pInstanceData, DWORD 
cbInstanceData, LPVOID pPropData, DWORD cbPropData)
+{
+   return E_NOTIMPL;
+}
  

R. Shearman: If this is not implemented in native, then a comment here instead stating this would be appropriate.

S_FALSE

 static HRESULT WINAPI Mediacontrol_Pause(IMediaControl *iface) {
     ICOM_THIS_MULTI(IFilterGraphImpl, IMediaControl_vtbl, iface);
+    TRACE("(%p/%p)->()\n", This, iface);
 
-    TRACE("(%p/%p)->(): stub !!!\n", This, iface);
+    if (This->state == State_Paused) return S_OK;
+
+    SendTheFilterAMessage(iface, SendPause);
+    This->state = State_Paused;
 
     return S_OK;
 }

C. Costa: Shouldn't S_FALSE be returned in the last statement?


E_FAIL

> [quoted text muted]
>+      if (!bTest) return E_FAIL;

R. Shearman [Nov 05]: Don't use E_FAIL. Try to return a proper error code, preferably using what native returns in this circumstance by adding a test case.


[quoted text muted]
>+      ERR("Failed to SetupOpenInfFileA(%s)\n", inf_name);
>+      return E_FAIL;
 

R. Shearman [Nov 05]: Again, don't use E_FAIL. Also consider adding some more debugging information to the error message (like, say, the value returned by GetLastError()). wine archive

When the programmer pointed to MSDN documentation which mentioned E_Fail, R. Shearman explained further: MSDN is [not always] correct [...] The best way to prove it either way is to write a test case that exercises this code path. Where this is not possible then you should return as appropriate an error message as possible and this will typically not be E_FAIL.

Undocumented Functions with Windows

F. Richter said [July 05]: I'm a bit hesitant to add GdiGradientFill to wingdi.h as this function is practically undocumented, it doesn't appear on MSDN or the Platform SDK's wingdi.h.

D. Timoshkov: Well, if you inspect Wine headers (or better Wine source) [...], you will find that it has a lot of undocumented things. There should not be any hesitation at all IMO in adding a prototype for already implemented API. Wine Archive

Undocumented Shell32

A programmer: I have stumbled upon a unimplemented function shell32.dll.SHLocalAlloc .

R. Shearman: [Aug 2005] What program are you running and which platform is it designed to run on? I ask because the win9x shell32 exports SHLocalAlloc at a different ordinal to the NT shell32. Our shell32 mostly has the ordinals for the win9x shell32, so if the program is designed to run on NT then the stub you are hitting is really something else. Wine Archive

Exception Handlers

_TRY

J. Lang [Jun 05] : 'Any restrictions on __TRY/__EXCEPT I should know about?'

R. Shearman: Wine Archive exception handlers can be nested. [Also]

From include/wine/exception.h:

Working with Dlls

Automated Registering of a Dll for a clean wine

P.Vriens [Sept 06]: while I'm working on DllRegisterServer for wintrust, I wanted to make it 'autorun' when running on a clean system (through wineprefixcreate). My thought was to add an entry to the RegisterDllsSection part in tools/wine.inf but that doesn't seem to work. (regsvr32 wintrust.dll does work). [He then explained the details] 'make install' in the tools directory makes sure the correct wine.inf is used in wineprefixcreate.

Adding a New Dlls Stub

D. Riekenberg [May 2007]

  1. git must know the new dll:
    git update-index --add dlls/<yourdll>/Makefile.in
  2. run tools/make_makefiles

See the wiki: http://wiki.winehq.org/Developers-Hints#head-30a7f7f51ce0e42b4141749c9832bae9f7957260

You should not include any autogenerated stuff in your Patch, otherwise your patch will possibly not apply, when another dll was added. [wine devel May 2007]


A. Julliard [Sept 06] in reply to a patch that adds a stub implementation for the DLL clusapi and GetNodeClusterState: You need to add a DLL_WINE_PREATTACH handler when adding a stub dll

This was repeated in August 07 [winedevel] when a developer tried to submit a patch for [a] missing lib. A. Julliard: You need to write a proper spec file with all exported functions, and you need to handle DLL_WINE_PREATTACH in DllMain.

There are no exported functions (for now). So i guess an empty spec file is OK?

A. Julliard: there are exported functions in the native dll, you have to declare them as stubs.


A new wine programmer added a new dll [Sept 07 wine devel]. This patch provides a first skeleton version of bits.dll.

M. Stefanuic: thanks for the patch but could you please add also a DLL_WINE_PREATTACH case in DllMain()? That way Wine can prefer to load a native bits DLL (if one is available) until the Wine one gets usable. See for example the mscoree DLL for an example (dlls/mscoree/mscoree_main.c).

Dills that only Forward

A developer queried about two dlls: They both are dlls with just one function and that it is forwarded to odpnet.dll. If we add to the git tree these two spec files

dlls/dpnaddr/dpnaddr.spec:
stdcall DirectPlay8AddressCreate( ptr ptr ptr )
dpnet.DirectPlay8AddressCreate
dlls/dpnlobby/dpnlobby.spec:
stdcall DirectPlay8Create(ptr ptr ptr) dpnet.DirectPlay8Create

Shouldn't these do to have both dlls implemented natively in wine?

A. Julliard: Yes, except you also need to write makefiles, and probably version resources [wine devel Jun 07].

Loaded dlls

A. Julliard [wine devel Jul 07]: You should use module handles to identify dlls, not file names. You can use GetModuleHandle to find out if a dll is loaded already.

The develope queried this: if the calling application has loaded a dll containing a SIP already, calls CryptSIPLoad, unloads the dll, then calls a function via the SIP, it'll fail. If I want to make sure that doesn't happen, I should LoadLibrary myself, yes? Then I'm forced to search by name myself, am I not?

A. Julliard: Most likely what you want is do a LoadLibrary in all cases and don't bother to cache anything, the loader will take care of the refcount.

He replied: The reason I cache is that the number of LoadLibrary calls is potentially unknown. Each function can have its own DLL registered in the registry. In my previous implementation you rejected, I enforce that a SIP exists entirely within one DLL, as that's what CryptSIPAddProvider allows. By doing so, I know how many LoadLibrary calls I've done - one for each SIP. If I

  1. allow a separate dll per function, as the registry allows, and
  2. don't cache the dll names myself, I guess I must store the HMODULE for each function? The cache allows me to avoid that.

In this implementation, I cache each DLL, so that I only do one LoadLibrary per dll name. This has less memory impact than the straw man approach I think you suggest, and is (I guess) more readable than the one-dll-per-SIP approach.

A. Julliard: Multiple LoadLibraries on the same dll don't cost anything, they just increment the refcount. If you want to enforce that all functions use the same library you can still do it by checking the returned HMODULE.

Working with sockets

D. Kegel [Nov 06] I noticed several uses of select() on sockets in the wine source code. This will fail in programs that need more than FD_SETSIZE file descriptors if one of the sockets being selected on happens to have a value of FD_SETSIZE or higher. Glibc fixes FD_SETSIZE at 1024, and does not allow this limit to be changed. Wine also uses poll(), which does not suffer from this limitation. We might want to rewrite this code to use poll() instead of select() sometime. The suspect statements are:

dinput/joystick_linux.c: if (1>select(This->joyfd+1,&readfds,...
dinput/joystick_linuxinput.c: if (1>select(This->joyfd+1,&readfds,...
icmp/icmp_main.c: while ((res=select(icp->sid+1,&fdr,...
netapi32/nbt.c: r = select(fd + 1, &fds, ...
wininet/internet.c: if (select(nSocket+1,&infd,...
wininet/netconnection.c: if (select(connection->socketFD+1,&infd,...
ws2_32/socket.c: if( (highfd = select(highfd + 1, p_read,
p_write, p_except, timeoutaddr))

Instance of select() that don't use file descriptors (i.e. that are just for delay) are safe, as are instances in conformance tests. Might make a nice little project for a new wine hacker...

One Developer wondered if some were safe: they poll on a single file descriptor. but A. Julliard: A single file descriptor can still be above 1024. select() should really be avoided. [Some patches were sent in] wine archive


Working with the registry

+    /* @@ Wine registry key: HKLM\Software\Microsoft\WindowsNT\CurrentVersion\GRE_Initialize */ 
+   if(RegCreateKeyExW(HKEY_LOCAL_MACHINE, gre_initialize_reg_key, 
+    0, NULL, 0, KEY_ALL_ACCESS, NULL, &hkey, NULL) == ERROR_SUCCESS)

D. Timoshkov [Oc 06]: '@@ Wine registry key' should be used to mark Wine internal registry keys, which is not the case here. wine archive

Debug Channels and Statements

General Suggestions for Debug Traces

Generally Selecting the Right Trace

M. Hearn [May 05]WineArchives : MESSAGEs always appear. They're meant for end user critical information rather than developer info. If it's developer info either use TRACE/WARN.

Dimi Paun: Just use the regular FIXME/ERR/WARN/TRACE() as appropriate. There is no [need] to use:
if (TRACE_ON(crypt))

MESSAGE("foo\n");


Traces that allow for different threads

A programmer asked if there was something that doesn't prefix the hexdumps with the function name for easier readability in the crypt_report_func_input, since there [was] already a TRACE/FIXME call being made prior to it's call.

A. Julliard: No, you really want the prefix on all lines, otherwise it makes a big mess when multiple threads are printing things at the same time.Wine Archives


Traces vs Message Box

Mike Hearn: There [could be some] messages which should be message boxes. We'd have to LoadLibrary/GetProcAddress user32.

A. Julliard: Messages that the user needs to see have to be a message box, yes. But really if we are doing things right there shouldn't be any need for such messages. In the specific case [of dxdraw] I'd suggest letting winecfg take care of it, by not offering the ddraw dlls in the load order config,and/or issuing a warning when they are set to native.

Trace Formatting and Readability

+        case INTERNET_OPTION_RECEIVE_THROUGHPUT:
+        case INTERNET_OPTION_SEND_THROUGHPUT:
+            WARN("This request is not supported by native either\n");

R. Shearman: You should print out the option being requested, otherwise the "This" in "This request" is meaningless. Just printing the option number is fine. wine archive


Hex Formatting

[June 05] S. Krasuckus said: [it] would be nice to print "err" in the following format "err=0x%08lx"

A Wine Programmer asked: Why do you want to change that? I thought %08lx is pretty common in most of the Wine code.

S. Krasuckus:I just see two versions of formatting in Wine and want to get rid of one of it. Not a large difference which. OTOH, which one case is clearer when reading logs:

00000021 or 0x0000021 ? I prefer the latter one. Wine Archive


+    ok(hr == S_OK, "ScriptStringAnalyse Stub should return S_OK not %08x\n", (unsigned int) hr);

V. Margolen [Jul 06]:wine archive Don't cast hr, use proper format instead: ok(hr == S_OK, "ScriptStringAnalyse Stub should return S_OK not %08lx\n", hr);


dbg_printf("0x%08llx", addr->Offset);

A. Julliard [Jul 06]: %ll formats are not portable, please avoid them. [...] In traces we should use wine_dbgstr_longlong. In tests we don't want to use debug macros so we should use the %lx%08lx trick. wine archive

Use the existing macros

+announce_bad_opaque_data()
> +{
> +    wine_dbg_printf("CryptUnprotectData received the following pDataIn DATA_BLOB that seems to\n");
> +    wine_dbg_printf("have NOT been generated by Wine:\n");
> +}

A. Julliard: You should use the TRACE/FIXME macros here, not raw wine_dbg_printf.

Message

/* There are too many bugs in FreeType < 2.1.9 for bitmap font support */
if(!FT_IS_SCALABLE(ft_face) && FT_SimpleVersion < ((2 << 16) | (1 << 8) | (9 << 0))) {
pFT_Done_Face(ft_face);
WARN("Please, update FreeType library (>=2.1.9) -- font %s will not be load\n", debugstr_a(file));

D. Timoshkov[Aug 05]: If it's supposed to be a user visible message then it should use MESSAGE instead of a WARN.Wine Archive


Trace

M. Karcher [Apr 09]: You must not call trace from within the argument list of an ok() statement. ok()/trace() first sets a global variable to the current location, and then calls a printing function passing and evaluating the parameters. If one of them changes the global variable already set, the location printed is wrong http://permalink.gmane.org/gmane.comp.emulators.wine.devel/68589



+        if (WINE_TRACE_ON(crypt))
+            wine_dbg_printf("\tChecking Map Index %s\n", debugstr_w(wszIndexKey));

M. McCormack:You don't need the "WINE_TRACE_ON( )" check, because TRACE already does that for the default debug channel, so the following is the same:
TRACE("\tChecking Map Index %s\n", debugstr_w(wszIndexKey);

M. McCormack [Oct 05]: use TRACE instead of DPRINT [and] don't use %S in debug statements (with TRACE) it's not portable, use %s and debugstr_w(str) instead Wine Archive

Fixme

R. Shearman [Mar 05]:Print FIXME's for parts that aren't implemented, otherwise the next person that comes along to this function will spend hours trying to work out what isn't implemented, if they even realise this function is to blame. Archive Link

D. Kegel pointed out that a stub which would normally be a fixme would sometimes be set as trace [Jul 06]: turn it into a TRACE instead of a FIXME. [...] That is standard practice for extremely intrusive and frequent FIXME's, I think. It's a practical thing. wine archive

In such a case S. Dosinger suggested: why not print the fixme once

static BOOL warned = FALSE;
if(!warned) {
FIXME("Stub!\n");
warned = TRUE;
}

Removing FIXME

/* FIXME: Should we be storing these dwFlags or the creation ones? */
-  lpPData = DP_CreatePlayer( This, lpidPlayer, lpPlayerName, dwFlags,
+  /* I think we should pass creation flags, so we can distinguish sysplayers and not count them in the current
+     player total */ 
+  lpPData = DP_CreatePlayer( This, lpidPlayer, lpPlayerName, dwCreateFlags, hEvent, bAnsi );

A, Julliard: [Jun 07 wine devel] Every time someone uses "I think" in the code, usually this calls for a test case. This one should be rather straightforward to test.

The developer replied: I've used "i think" more as an answer to the previous FIXME comment we have inherited from the author of the code.

A. Julliard: If you have fixed the FIXME then you should remove it. The source code isn't the right place to start a discussion with the previous author ;-)

Implementing a Fixme

+/* AMStreamConfig interface, we only need to implement {G,S}etFormat */
+static HRESULT WINAPI AMStreamConfig_QueryInterface(IAMStreamConfig * iface, REFIID riid, LPVOID * ppv)
+{
+   FIXME("%p: stub, not worth the effort\n", iface);
+   return S_OK;
+}


R. Sheaman showed how to implement this using a FIXME:

 {
    if (IsEqualIID(riid, &IID_IUnknown) ||
        IsEqualIID(riid, &IID_IAMStreamConfig))
    {
        *ppv = (LPVOID)iface;
        return S_OK;
    }

    FIXME("No interface for iid %s\n", debugstr_guid(riid));
    return E_NOINTERFACE;
 }


Example Fixme - Unsupported by MS implementation

+        case INTERNET_OPTION_SEND_THROUGHPUT:
+            WARN("This request is not supported by native either\n");
+            SetLastError(ERROR_NOT_SUPPORTED);
+            break;

Dimi Paun: I'd suggest to use FIXME instead of WARN here. It doesn't really matter that current version of miscrosoft's implementation doesn't support them, they may add support for them later, and the apps will start to use them.


Example Fixme - Incomplete implementation

+	if (usernamelen > char_cnt) 	// there is more to do and not implemented
+	{
+	TRACE("Username len %i exceeds parsed text %i\n", usernamelen, char_cnt);

This should be a FIXME.


+static HRESULT WINAPI AMStreamConfig_SetFormat(IAMStreamConfig *iface,
+                                              AM_MEDIA_TYPE *pmt) {
+   ERR("%p: %p stub !!\n", iface, pmt);
+   return E_NOTIMPL;
+}


R. Shearman: Use a FIXME instead of ERR. Wine Archive Link


+static HRESULT WINAPI AMStreamConfig_GetNumberOfCapabilities(IAMStreamConfig *iface, int *piCount, int *piSize)
+{
+   TRACE("%p: %p %p - stub! Should be harmless\n", iface, piCount, piSize);


R. Shearman: This should be a FIXME. If that produces too much output then use a WARN instead.



Example Fixme - Readability

@@ -78,29 +77,35 @@ static HRESULT WINAPI PersistMoniker_IsD
 static HRESULT WINAPI PersistMoniker_Load(IPersistMoniker *iface, BOOLfFullyAvailable,IMoniker *pimkName, LPBC pibc, DWORD grfMode)
 {
-    FIXME("(%p)->(%x %p %p %08lx)\n", iface, fFullyAvailable, pimkName,pibc, grfMode);
+    PERSISTMON_THIS
+    FIXME("(%p)->(%x %p %p %08lx)\n", This, fFullyAvailable, pimkName,pibc, grfMode);
     return S_OK;
 }


Dimi Paun [Jun 05] This code looks weird. It should still look like C code, even if we use a macro. I would suggest something like this [...]It's a bit more verbose, but a lot more readable: Wine Archive

static HRESULT WINAPI PersistMoniker_Load(IPersistMoniker *iface, BOOL fFullyAvailable,IMoniker *pimkName, LPBC pibc, DWORD grfMode) {

HTMLDocument *This = PERSISTMON_THIS(iface);
FIXME("(%p)->(%x %p %p %08lx)\n", This, fFullyAvailable, pimkName, pibc,grfMode);
return S_OK;

Err

Dimi Paun [May 05]: ERR() should be called to signal an internal error such as inconsistent state, not something that [a] function is designed to handle. [If] function knows how to deal with [a] condition gracefully, just get rid of the ERR(). [...]

We should never generate an ERR() only based on a user setting. [...or] a reasonable user configuration, it should be handled gracefully.


> +         if (!capBox->grab_data)
> +         {
> +            ERR("%p: Out of memory?\n", capBox);
> +            return E_OUTOFMEMORY;
> +         }

Dimi Paun: Ditto. Also the code is more clear if you just do:

if (!capBox->grab_data) return E_OUTOFMEMORY; Wine Archives


else
+    {
+	TRACE("CreateFileMoniker Failed %04x\n", (int) rc);	

[This should be an] ERR


+    is_result_ok(FALSE);

V. Margolen [Sept 05]: This is not helpfull really. Indicate what failed.


+    if (!HIToolBoxDLHandle) {
+        fprintf(stderr, "%s impossible d'ouvrir HIToolBoxDLHandle\n", __FUNCTION__);
+        return nil;

A. Julliard [Nov 06]: you should use ERR instead of fprintf, and error messages should be in English <g>

Todo

A. Julliard: A todo is supposed to mark something that will need to be addressed later on.

A programmer asked how a Windows bug should be handled, and whether it should be marked with a todo.

A. Julliard: Obviously we can't fix the Windows code, so that's not a todo, it's just a behavior of the API that should either be handled by the test, or simply not tested at all if the results are not reliable.

The programmer asked Surely, it is also good to document any bugs we find in Windows, and, though perhaps not for NT4, Microsoft *may* fix them in the future. Why not rename "todo" to "bug_in" ? [the discussion ended there] Wine Archives


+todo("windows")
+{   /* XP returns COMPLEXREGION although dump_region reports only 1 rect */
+    ok(ret == SIMPLEREGION, "IntersectClipRect returned %d instead of SIMPLEREGION\n", ret);

A. Julliard: It doesn't make sense to add a todo for Windows, we won't be able to fix the bug there...

A programmer thought: that's how we can see if the bug is fixed in the future windows versions.

A. Julliard: That's not what the todo mechanism is for. The purpose of the regression tests is not to debug Windows, it's to capture the Windows behavior, no matter how broken it may be. We can't add todos just because we don't like the way an API behaves; we have to deal with it.wine archive


A programmer noted: Today writing a testcase I found a "problem" on todo_wine construct. In short :

void first_test_part()
{
ok(test1(), "failed\n");
}
void second_test_part()
{
ok(test2(), "failed\n");
}
todo_wine
{
first_test_part();
second_test_part();
}

this "fails" (i.e. is marked as passing inside todo_wine block) if first_test_part() passes, which is not what wished; in this case the test should be considered passing if BOTH first and second part passes, and failed otherwise. The problem arose on bitmap creation test (gdiplus) on which the testcase should check for creation success AND bitmap internal [consistency]. My example is short, but in my case there's a single function used to test many kind of bitmaps; that's impossible to do with todo_wine actual construct. It could be obviously separated into many functions, each enclosed on its own todo_wine, but this would vanify the "single test function for many bitmaps types" way.

V. Povirk: [May 09] This is by design. Each call to ok() is an individual test, independent of the others, and todo_wine marks them all as todo. If you have the same code running multiple tests, you can mark some of them as todo by putting a todo flag in your data. If you really want multiple checks to be dependent, you'll need to reduce them to one ok() call.

Working with Linux Versions

The FreeDesktoop Standard

Changelog: wineshelllink: Use FreeDesktop standard to create Wine menu structure.

J. White [Nov 06]: I would strongly recommend that this be done instead with xdg-utils: http://portland.freedesktop.org/wiki/XdgUtils

That will allow us to avoid the pain of maintaining our own implementation, and allow distro vendors to make their distro work with Wine just by supplying a working version of xdg-utils. The trick is whether or not we can persuade Alexandre and/or all of the Wine packagers to include xdg-utils by default in distributions.

K. Krammer: In case there are any questions on xdg-utils, I am right here (i.e.subscribed to wine-devel)

A. Julliard: If xdg-utils can be used, then that seems clearly preferable to reinventing them ourselves. Even if it's a bit more effort in the short term it should save us trouble in the long run.

Windows Version

A programmer queried dealing with differences in Window versions: The behavior of NT and W2K/WinXP differ a lot for SystemTimeOfDayInformation.

The struct for NT is 32 Bytes compared to 48 for the others. Apart from that returncodes etc.. are totally different. The included test explains (some of) the differences. The test now also caters for all the above mentioned versions, as does nt.c (that means setting the wine version to nt4 produces the correct results). nt.c now has a hard coded value for the size of the struct for NT (32).

A. Julliard: [Jun 05]You should not do that kind of version-dependent code unless there is an app that requires it. Otherwise just implement the XP behavior,which should be backwards compatible anyway.Wine Archives

Working with the Registry

 +        r = RegCreateKeyExW(hkeyMap, wszIndexKey, 0, NULL, REG_OPTION_NON_VOLATILE, KEY_ALL_ACCESS, NULL, &hkeyOpen,
&dwDisposition);
 +        if (r != ERROR_SUCCESS)
 +           continue;
 +        if (dwDisposition == REG_OPENED_EXISTING_KEY)
 +        {
 +            /* already exists, skip */
 +            CloseHandle(hkeyOpen);

D. Timoshkov:Registry keys should be closed by RegCloseKey, not CloseHandle (here and everywhere else).

Set Last Error

A programmer asked about whether to do something extra to preserve the LastError.

A. Julliard: If an app depends on it, yes. [...] The thing that matters is setting the proper error codes on errors. Setting or not setting last error on success very rarely matters, and both on Windows and Wine it will usually simply be a side-effect of the functions being called internally.

If we wanted to replicate the exact Windows behavior we'd essentially need to manipulate last error in every single code path, and that would be a real nightmare; so we should only do it if there's a demonstrated need. wine archive

A. Julliard [Aug 06]: In general, testing that last error isn't set on success is not useful, unless you really have an app that depends on it. wine archive


 +    protected = CryptProtectData(NULL,desc,NULL,NULL,NULL,0,&cipher);
 +    ok(!protected, "Encrypting without plain data source.\n");
 +    r = GetLastError();
 +    ok2(r == ERROR_INVALID_PARAMETER, "Wrong (%lu) GetLastError seen\n",r);

D. Timoshkov:If you are going to test last error value after an API call it's a usual practice to set the error first to some invalid value, 0xdeadbeef works fine. Wine Archive Link

+++ b/dlls/kernel/tests/heap.c [...]
+    SetLastError(MAGIC_DEAD);
+    hsecond = GlobalFree(gbl);      /* invalid handle: free memory twice */

D. Timoshkov [Mar 06]: There is no need to invent a new magic number, a usual convenience for Wine tests is to use 0xdeadbeef. wine archive

Class Factories

+ * DirectShow ClassFactory
+ */
+typedef struct {
+    IClassFactory ITF_IClassFactory;
+
+    DWORD ref;
+    HRESULT (*pfnCreateInstance)(IUnknown *pUnkOuter, LPVOID *ppObj);
+} IClassFactoryImpl;

M. McCormack:ClassFactories don't need to be allocated and freed, so it's sufficient to use a statically allocated IClassFactoryImpl, where AddRef and Release return 2 and 1 respectively.


MSI

 +    rc = MSI_DatabaseOpenViewW(package->db, ExecSeqQuery, &view);
 +    if (rc != ERROR_SUCCESS)
 +        return ERROR_SUCCESS;
 +
 +    rc = MSI_IterateRecords(view, NULL, ITERATE_PublishComponent, package);
  
      return rc;
  }

M. Hearn: You need to release the view you opened. MSI_IterateRecords leaves the view intact (it does not call msiobj_release). Wine Archive Link

Unspoken 'Suggestions'

Often projects have methods that become like a standard, an unspoken rule. You can list them here:

Feb 2005

A. Mohr: pre-processor directives (#ifdef, ...) always start at the very beginning of a line to make sure one recognizes immediately which parts of the code are being processed and which ones aren't.

M. Hearn: In short:

  1. No C++ style comments
  2. No inline variable declarations (in the middle of a block)
  3. Can't use L"" string syntax
  4. No space between # and directive

Basically old-style C conformance + a few random bits for non-GNU compilers. Hmm, a few others that I am probably forgetting. There aren't that many rules. Archive Link

M. McCormack: Renaming files in CVS should be avoided as much as possible. It makes patch management (doing diffs and reverting patches) difficult. Archive Link

D. Timoskov: It's better to use W versions of APIs even if it's not strictly required. You shouldn't use string APIs supposed to handle ASCII strings. Use lstrcpyW,lstrcatW and lstrlenW in this and all other cases. Before printing strings on console, you need to convert unicode strings to the encoding of the underlying system. Archive Link

Odds Bodds, Stuff You dont yet know where to put

Parsing

In a discussion about improving wine's cmd parsing, several suggestions were made about different options for parsing. However D. Kegel [Dec 09] noted: we already have eight parsers written in bison in wine. Archived-At: http://permalink.gmane.org/gmane.comp.emulators.wine.devel/74742

Error Messages, Wine and copyright

jul 09 someone asked: Is there some kind of copyright on the resources? Are we allowed to use the same strings or should it always be our own words (like with comments in the source)?

A. Julliard: As far as possible it should be our own words. In general there's no reason to copy the exact wording of error messages.

Unix file paths

why does wine support UNIX paths? What is the circumstance where a Windows application will be trying to access a native file or directory? The only example I can think of is that an app has specifically been written to be used in Wine, in which case, shouldn't native UNIX paths be disabled by default, and perhaps turned on with an environment variable?

A. juliard apr 09: It can be used anywhere an app uses a user-specified path without mangling it too much; admittedly that doesn't happen very often, apps like to mangle paths. There are also places where Wine itself depends on it, to support things like "wine ~/foo.exe" or to allow Unix paths in some registry entries. These are all things that could probably be reimplemented in a more reliable fashion, for instance by using \\?\unix instead of relying on the path detection heuristic. Once this is done properly everywhere, then maybe the hackish way could be removed.

tips for your favourite Editors

Eclipse

S. Rose [wine devel Aug 07]: Eclipse 3.3 with CDT has an automatic code formatting feature, Ctrl +Shift+F, [...you can] load some files into that and run that over the file to instantly clean up stuff like that. You can tweak the code style to your liking too.

K. Blin cautioned about formatting existing code: Formatting-only patches usually aren't accepted.

Wineprefix comments and friends

A programmer wrote some code for wineprefix: create a directory for the menu and a configuration file that enables this menu in the freedesktop.org menu structure. As suggested on wine-devel the menu directory is a subdirectory of ~/.wine, so if the user deletes it, all menu items created there will be automatically removed.

A. Julliard: That sort of desktop-specific code does not belong in wineprefixcreate.I'm not really convinced that this approach to menus is correct, but even if it is, it should be handled by wineshelllink and friends. wine archive

Code Tools with Wine

Windows API check

A developer noted in a code review [Jul 06]:

+ * MsiGetLanguage (MSI.@)
+ */
+UINT WINAPI MsiSetInstallLevel(MSIHANDLE hInstall, int iInstallLevel)

[that is a] typo. Do we have a tool that sweeps the source for doc - code mismatches?

M. Stefanuic: Yes, tools/winapi/winapi_check. [wine archive http://www.winehq.org/pipermail/wine-devel/2006-July/049659.html]

Smatch

Smatch is a static analysis tool which tries to find bugs in C source code. It tends to find a lot of false positives unfortunately but it also does find some actual bugs.

Here are the instructions to use it with wine. You will, of course, have to change the path names:

git clone git://repo.or.cz/smatch.git cd smatch
make
cd /path/to/wine/
make clean
make CHECK="/path/to/smatch/smatch -p=wine --full-path" CC=/path/to/smatch/cgcc 2>&1 | tee warns.txt

That will run smatch over the entire source, but if you just want to quickly test one file the command to use is:

 /path/to/smatch/smatch_scripts/wine_checker.sh your/file.c

Winetestbot

A developer asked: can somebody tell if there is something obviously wrong with the example[..]

U. Bonnes: write [tests] for the test suite. And for trying genuine windows, use the winetsetbot. No need to reboot. Register with the winetestbot, upload your test exe and compare the test output

GUID

D. Riekenberg Found a big online GUID-List [Jul 06]:

COM

R. Shea [wine devel Nov 07]: [...] In an attempt to help other new Wine and COM developers I wrote a tutorial on COM development the Wine way. This tutorial describes:

with instructions on Wine integration and, for Windows users or testers building for Windows, basic instructions on building and running these same applications in Windows. The tutorial is available at: http://ambleramble.org/wine/

I hope other developers enjoy this documentation. If you see mistakes, either minor typos or conceptual problems, please drop me an email and I'll clean up the docs.

RPC

A developer asked R. Shearman feb09: could you suggest a good documentation source on RPC? Sure, although this particular aspect isn't well documented. I did find a couple of references, but you have to fill in the blanks to get to the conclusion that RpcServerUseProtseqEpEx with a NULL endpoint should generate an otherwise unused endpoint string:

The following may also be useful in future RPC issues, but not in this case:


One visiting programmer noted feb 09: see implementation in dual BSD-licensed _and_ LGPL-licensed freedce reference implementation, on which MSRPC was originally blatantly copied (thanks to the BSD license on the 1.0 reference implementation). e.g. line 2470 online annotated copy here: http://www.sfr-fresh.com/linux/misc/freedce-1.1.0.7.tar.gz:a/freedce-1.1.0.7/ncklib/com/comnet.c


anyone who is doing wine dce/rpc development really needs to be looking at the reference implementation, side-by-side, line-for-line, in virtually every aspect except for exception handling, threading, binary-interoperability with the typelibraries and a few other areas.Archived-At: <http://permalink.gmane.org/gmane.comp.emulators.wine.devel/67050>

MSDN

MSDN is not always accurate. One programmer queried the mailing list: I noticed MSDN listing DsGetDcName as taking 7 parameters (there's an extra domain name as 3rd parameter) while wine versions have only 6 parameters. Which is correct?

V.Margolen: SDK is correct. And it has only 6.wine archive

D. Kegel:Lately, when I search for win32 API functions like CoGetObject, the top few hits I get at microsoft.com are the .net ones. I guess it's not a problem [...]

I. Leo Pouti: Calling everything with a new name is an easy way to get people to think you've got something new, when you haven't. They could call them "the .net win32 API" and some people may actually think it's new stuff. wine archive

Windows Messages

atoi

where would atoi() function be linked now to?

H. Leidekker: The native C library (glibc on Linux). Wine Link May05


Native Dll vs Builtin Dll

Loading a Dll as a Datafile
A Programmer noted: Wine causes loading of the native dll instead of builtin when you load it as a data file, so I tested the builtin dll with native resources.

A. Julliard explained: That's a feature ;-) We can't support loading builtins as datafiles, so the idea is that it's more likely that the app will be happy with loading the native as datafile than loading the builtin in normal mode. It does make the behavior a bit surprising I agree. Wine Archive


detect cdrom

     /* create entries for cdroms (skipping A: and B:) */
     for (i = 2; i < 26; i++)
     {
         drive[4] = 'A' + i;
+        drive1[0] = 'A' + i;
+        if (GetDriveTypeA(drive1) != DRIVE_CDROM) continue;
         handle = CreateFileA( drive, 0, 0, NULL, OPEN_EXISTING, 0, 0 );
         if (handle == INVALID_HANDLE_VALUE) continue;

A. Julliard [July 05]: You shouldn't use GetDriveType here, you should determine the device type from the ioctls you do on the handle. The GetDriveType result doesn't necessarily correspond to the device since it can be changed in the registry. Wine Archive

Personal tools
Namespaces
Variants
Actions
Navigation
Toolbox