Re: gdi32: "StretchDIBits" with value of zero for "xSrc" and "ySrc"
Hi Mathias, + * mkosch (2008/09/22) Please don't put such comments in source files, the source files aren't a changelog. + hProcessHeap = GetProcessHeap(); + assert(hProcessHeap != NULL); This assert is pointless, GetProcessHeap cannot reasonably fail. And more generally.. + assert(hDC != NULL); dont' use assert() in the tests, especially for conditions such as this which have nothing to do with the function you're testing. These will only fail in out-of-memory type situations, and many other things will fail before your particular test. So either assume that such basic functions will succeed, or use ok to check them if there's some reasonable probability they may fail, or use a combination of skip and return if one fails. Just please don't use so many asserts in the tests. + // Initializing random number generator: In the first place, such a comment is unnecessary, is it's quite clear what the following line does. In the second place, C++-style comments are not allowed in Wine source. --Juan
"Juan Lang" <juan.lang(a)gmail.com> writes:
+ // Initializing random number generator: In the first place, such a comment is unnecessary, is it's quite clear what the following line does. In the second place, C++-style comments are not allowed in Wine source.
More importantly, a test should not be using random data. We have already enough trouble with sporadic test failures, we don't want to add deliberate randomness. -- Alexandre Julliard julliard(a)winehq.org
Hello Juan! Thanks for your valuable comments.
+ * mkosch (2008/09/22) Please don't put such comments in source files, the source files aren't a changelog. Is there a right place for such comments? Since I didn't yet prove if this code is buggy and also don't want to investigate further time in this (unless practical problems occur for me) Bugzilla is the wrong place. (Note: I think there should be problems i.e. with XOR, because this code appears to XOR the destination bitmap with gaps and then again with the destination bitmap, leaving the gaps.)
+ hProcessHeap = GetProcessHeap(); + assert(hProcessHeap != NULL); This assert is pointless, GetProcessHeap cannot reasonably fail. And more generally.. According to MSDN [1] there is a chance that "GetProcessHeap" might fail. It's bad style to expect what doesn't always have to happen.
+ assert(hDC != NULL); dont' use assert() in the tests, especially for conditions such as this which have nothing to do with the function you're testing. [...] or use ok to check them if there's some reasonable probability they may fail
Vitaliy Margolen responded to my previous version of this patch:
+ if (!pBitmapData) + ok(0, "Memory allocation failed.\n"); Don't do this. If allocation failed assert or skip the test entirely.
I'm a little confused now, because he told me to not use "ok" but suggested to assert.
So either assume that such basic functions will succeed, I may do this for something less important like a test case, although I still think it's not good style.
or use a combination of skip and return if one fails. I will try once more, this time using "skip" instead of assertions.
In the second place, C++-style comments are not allowed in Wine source. Sorry I missed this one out. I didn't notice any compiler warnings.
[1] http://msdn.microsoft.com/en-us/library/aa366569(VS.85).aspx -- Mathias Kosch <info(a)mkosch.de> | http://www.mkosch.de/ RSA Public Key: http://www.mkosch.de/pubkey Einfach zu bedienende Linux-Distribution: http://www.opensuse.org/
Hi Mathias,
+ * mkosch (2008/09/22) Please don't put such comments in source files, the source files aren't a changelog. Is there a right place for such comments?
I meant your name and the date in particular. The rest of the comment is probably okay, though a FIXME would be helpful to indicate something not totally understood.
According to MSDN [1] there is a chance that "GetProcessHeap" might fail. It's bad style to expect what doesn't always have to happen.
Matters of style are always debatable, but in the Wine tests, we generally expect such things to succeed. As I noted elsewhere, a strange condition such as GetProcessHeap failing will result in many, many things failing, so being defensive in this case doesn't gain much except additional complexity in the tests. GetProcessHeap only fails if the Wine environment is not properly set up, and this really can never happen in the tests. When in doubt, read the source! MSDN isn't as helpful here.
Vitaliy Margolen responded to my previous version of this patch:
+ if (!pBitmapData) + ok(0, "Memory allocation failed.\n"); Don't do this. If allocation failed assert or skip the test entirely.
I'm a little confused now, because he told me to not use "ok" but suggested to assert.
I'm sorry, I didn't note Vitaliy's prior response. I think we're both reacting to the defensiveness of your tests. Defensive programming in general may be a good thing, though there is some debate on this.. but I digress. In the Wine tests, if memory allocation begins to fail, many, many tests will fail, so the defensive style doesn't gain anything except additional code complexity. We wish to avoid that, so in general, we expect things like memory allocation to succeed in the tests. So, to combine my advice and Vitaliy's: just don't check if memory allocation succeeds or fails in the tests. Assume it will succeed.
In the second place, C++-style comments are not allowed in Wine source. Sorry I missed this one out. I didn't notice any compiler warnings.
No, you wouldn't notice any on a recent version of gcc, as it will silently accept C++ comments unless you specify -fpedantic or some such. The trouble is, not everyone uses a recent version of gcc, and their builds will fail with C++ comments left in. --Juan
Mathias Kosch wrote:
+ assert(hDC != NULL); dont' use assert() in the tests, especially for conditions such as this which have nothing to do with the function you're testing. [...] or use ok to check them if there's some reasonable probability they may fail
Vitaliy Margolen responded to my previous version of this patch:
+ if (!pBitmapData) + ok(0, "Memory allocation failed.\n"); Don't do this. If allocation failed assert or skip the test entirely.
I'm a little confused now, because he told me to not use "ok" but suggested to assert.
Don't use assert for every single thing. In your test things like GetDC(NULL) and CreateCompatibleDC(hDC) just have to succeed. If they don't something is really really wrong. And the whole test will just crash. So for those things using assert is fine. Memory allocations should "just work". No need to test for that. Or if you really want to just gracefully skip the entire test in this case. But only if it doesn't add too much complexity. Tests are made to test one thing at a time. For calls that are related to what you testing and that should succeed like StretchDIBits() and GetDIBits() just use ok() to test their result.
+ for (x = 0; x < nWidth; x++) + { + if ((pStartLine[x+xSrc].rgbtBlue != pNewStartLine[x+xDest].rgbtBlue) || + (pStartLine[x+xSrc].rgbtGreen != pNewStartLine[x+xDest].rgbtGreen) || + (pStartLine[x+xSrc].rgbtRed != pNewStartLine[x+xDest].rgbtRed)) + { + } Just use memcpy here.
Vitaliy.
participants (4)
-
Alexandre Julliard -
Juan Lang -
Mathias Kosch -
Vitaliy Margolen