Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Nikolay Sivov [EMAIL PROTECTED] writes: I don't think so. If the call fails testing return value doesn't make any sense. In this case 'result' is uninitialized and remains uninitialized after a call if it fails (here I mean a native too). So why should we check something which has unpredictable value? By the way first time I saw this here 8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c.. The difference is that this was a todo_wine, so it's expected to fail and we don't want to test uninitialized data since that can cause random test failures. In your case the test is expected to succeed, so the only way you'll get uninitialized data is when the first test fails, and then it doesn't matter if we get a random second failure, the test failed already. -- Alexandre Julliard [EMAIL PROTECTED]
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Alexandre Julliard wrote: Nikolay Sivov [EMAIL PROTECTED] writes: I don't think so. If the call fails testing return value doesn't make any sense. In this case 'result' is uninitialized and remains uninitialized after a call if it fails (here I mean a native too). So why should we check something which has unpredictable value? By the way first time I saw this here 8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c.. The difference is that this was a todo_wine, so it's expected to fail and we don't want to test uninitialized data since that can cause random test failures. In your case the test is expected to succeed, so the only way you'll get uninitialized data is when the first test fails, and then it doesn't matter if we get a random second failure, the test failed already. I see. I've just reposted it with 'try3' label, there's no linking based on return value now. Now I'm explicitly initialize output before each call to be sure that values changed or not. What do you think about matrix comparison or in general float comparison implementation? What is the best way to implement this. We have two cases now: 1. GdipIsMatrixEqual which compares two matrices (with simple bitwise operation now) 2. GdipIsMatrixInvertible which contains a check for 'not above zero' determinant. I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that?
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Am Sonntag, den 13.07.2008, 12:42 +0400 schrieb Nikolay Sivov: I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that? That might a good approach if we want to implement some new API and want to do it sensibly. That's the point where design questions are asked. But the Wine project works the other way round: Wine is bug-for-bug compatible with Microsoft Windows. This means we have to design the same way as Microsoft, whether we think this behaviour is idiotic or not. [Next paragraph is generally about Wine development. I do not imply that you are going to violate the rules stated here or already have sone so. Its just for your (or everybody who reads this mail in the archive) information] So: First you try out what native does. And I mean *try* *out*, not reverse engineer by disassembling. To proof that you have tried out what native does, you publish your tests as wine testcases. Then you implement wine in a way these testcases pass. For myself, I think it is acceptable to implement some API without comparing to native first, if you get an application to run by that, *but* to get this patch included, you better write an API test that shows how that application used the API you changed (or implemented) and verifies that your implementation not just happen to make the application run, but really returns the same result as native does. If you post a patch, always be prepared to be asked to test further cases. It greatly increases your chance to get your patch committed if you add these further cases (often, they show deficiencies in your patch then). Modify your patch to make these tests pass too, or (with a convincing explanation why this would be out-of-scope for your patch, for example needing to implement a whole new thing to make it pass) include the test as todo_wine. Do never ignore a test request because your implementation of an API fails it. As explained above: The point What nativce does is an extremely stupid way to handle these corner cases, my implementation if much smarter about it is never valid for Wine. I made the explicit note about disassembling (be it with IDA, w32dasm or getting an assembler listing of Microsoft dlls with some debugger), because for legal reasons, developers that have seen Microsoft source or object code are not allowed to commit code to Wine anymore. IIRC there is no consensus yet to what extent this rule applies, but (IIRC again) the usual interpretation is you may not contribute to the dll you disassembled. Regards, Michael Karcher
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Reece Dunn wrote: 2008/7/13 Nikolay Sivov [EMAIL PROTECTED]: What do you think about matrix comparison or in general float comparison implementation? What is the best way to implement this. We have two cases now: 1. GdipIsMatrixEqual which compares two matrices (with simple bitwise operation now) Is this the way it works on Windows? You have 4 basic cases: a. exactly equal; b. equal + epsilon for rounding/representation errors; c. equal - epsilon for rounding/representation errors; d. not equal. The code at the moment covers a and d (is there a test for d?), but what about b and c? These would be useful, because if the Windows version of GdipIsMatrixEqual *is* catering for error of a given epsilon, then you can use that function for validation. If not, you have tests that prove that Windows is using a simplistic comparison method. Exactly, so I plan to test this deeper on native. As I see now after some basic tests, most probably native call does a bitwise comparison cause the result is affected by a smallest matrix element change (e.g. 1.000f - 1.001f). But I'll test it more. Reece, do you have any suggestions to do some bounds test (I mean test is call affected by representation or round errors or not)? 2. GdipIsMatrixInvertible which contains a check for 'not above zero' determinant. It has been a while since I have done any matrix math. I'll assume that the above is the correct definition/check for invertible (makes sense because you have inv(a(ij)) = a(ij)/det(A), so can't have det(A) == 0). Now here, you have several 'classes' of matrices w.r.t. whether they are invertible or not: a. det(A) 0 b. det(A) == 0 c. det(A) 0 So does Windows match your assumption for the answer to these (a == yes; b, c == no). What do you mean here by (c = no)? Native allows negative det and I do so. I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that? Are you saying that GdipIsMatrixInvertible should be used for comparison of two matrices? If so, you can have two different matrices that are invertible, so it does not make sense. Where did I say that? It's weird. A good first step would be to print out the expected and returned values for the matrices, and keep the GdipIsMatrixEqual check. This will allow you to determine if the reason this is failing is due to rounding errors, or that they are completely different results. - Reece With last patch marked 'try3' http://www.winehq.org/pipermail/wine-patches/2008-July/057797.html test and crosstest work for me cause there aren't affected by representation errors using det == 16..
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Michael Karcher wrote: Am Sonntag, den 13.07.2008, 12:42 +0400 schrieb Nikolay Sivov: I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that? That might a good approach if we want to implement some new API and want to do it sensibly. That's the point where design questions are asked. But the Wine project works the other way round: Wine is bug-for-bug compatible with Microsoft Windows. This means we have to design the same way as Microsoft, whether we think this behaviour is idiotic or not. [Next paragraph is generally about Wine development. I do not imply that you are going to violate the rules stated here or already have sone so. Its just for your (or everybody who reads this mail in the archive) information] So: First you try out what native does. And I mean *try* *out*, not reverse engineer by disassembling. To proof that you have tried out what native does, you publish your tests as wine testcases. Then you implement wine in a way these testcases pass. For myself, I think it is acceptable to implement some API without comparing to native first, if you get an application to run by that, *but* to get this patch included, you better write an API test that shows how that application used the API you changed (or implemented) and verifies that your implementation not just happen to make the application run, but really returns the same result as native does. If you post a patch, always be prepared to be asked to test further cases. It greatly increases your chance to get your patch committed if you add these further cases (often, they show deficiencies in your patch then). Modify your patch to make these tests pass too, or (with a convincing explanation why this would be out-of-scope for your patch, for example needing to implement a whole new thing to make it pass) include the test as todo_wine. Do never ignore a test request because your implementation of an API fails it. As explained above: The point What nativce does is an extremely stupid way to handle these corner cases, my implementation if much smarter about it is never valid for Wine. I made the explicit note about disassembling (be it with IDA, w32dasm or getting an assembler listing of Microsoft dlls with some debugger), because for legal reasons, developers that have seen Microsoft source or object code are not allowed to commit code to Wine anymore. IIRC there is no consensus yet to what extent this rule applies, but (IIRC again) the usual interpretation is you may not contribute to the dll you disassembled. Regards, Michael Karcher All of this is absolutely clear. The only reason why my last tests fail was that I didn't check them on native thinking that a crossbuild is much complicated procedure that it is in fact. Of course I will not reverse any Windows binary to look at it cause it's a direct License Agreement violation. With this question I didn't try to bring some incompatibility with a 'better' implementation, not equivalent with native. Of course not. I've just asked the possible way to start with this problem of portable floating point comparison. If I'll found that native use some 'not bitwise' comparison for this calls I'll have to use some comparison method with tolerance value(s) and then try to get the same results as native provides. The question was 'what method is preferable in this case' that's all.
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Am Sonntag, den 13.07.2008, 13:51 +0400 schrieb Nikolay Sivov: Reece Dunn wrote: Is this the way it works on Windows? You have 4 basic cases: a. exactly equal; b. equal + epsilon for rounding/representation errors; c. equal - epsilon for rounding/representation errors; d. not equal. I don't see a difference between b and c. A matrix has four elements. Some might be slightly too big and others slightly too small. Would this make b or c? It's just one case: equal +/- epsilon. Exactly, so I plan to test this deeper on native. As I see now after some basic tests, most probably native call does a bitwise comparison cause the result is affected by a smallest matrix element change (e.g. 1.000f - 1.001f). But I'll test it more. 1+FLT_EPSILON is the value to use for the smallest matrix element change. But sour test already is a very strong indication that it really is strict equality test. FLT_EPSILON is in float.h Reece, do you have any suggestions to do some bounds test (I mean test is call affected by representation or round errors or not)? Another thing you could test is: Is the Matrix (1,FLT_EPSILON/2,0,1) equal to (1,0,0,1). A matrix like te former can easily be the result of multiplying a matrix by its inverse matrix (if represented as floating point numbers). One might expect that the product of a matrix and its inverse always is the identity matrix. 2. GdipIsMatrixInvertible which contains a check for 'not above zero' determinant. a. det(A) 0 b. det(A) == 0 c. det(A) 0 So does Windows match your assumption for the answer to these (a == yes; b, c == no). What do you mean here by (c = no)? Native allows negative det and I do so. He implied it form your not above zero quote. A hint how I would implement that test (if it turns out that a strict comparison to 0 is not the right result): Do not calculate the sum of a*d and -b*c, but compare these numbers in a sensible way. Regards, Michael Karcher
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Am Sonntag, den 13.07.2008, 14:09 +0400 schrieb Nikolay Sivov: Michael Karcher wrote: Am Sonntag, den 13.07.2008, 12:42 +0400 schrieb Nikolay Sivov: I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that? [ Deleted quotes about how to work on Wine ] All of this is absolutely clear. OK. Won't tell it to you again. With this question I didn't try to bring some incompatibility with a 'better' implementation, not equivalent with native. You have written that we should choose an appropriate method. We can not choose an appropriate method. Microsoft has chosen one. We have to find out which. And it might be different for GdipIsMatrixInvertible and GdipIsMatrixEqual. Regards, Michael Karcher
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
2008/7/13 Nikolay Sivov [EMAIL PROTECTED]: What do you think about matrix comparison or in general float comparison implementation? What is the best way to implement this. We have two cases now: 1. GdipIsMatrixEqual which compares two matrices (with simple bitwise operation now) Is this the way it works on Windows? You have 4 basic cases: a. exactly equal; b. equal + epsilon for rounding/representation errors; c. equal - epsilon for rounding/representation errors; d. not equal. The code at the moment covers a and d (is there a test for d?), but what about b and c? These would be useful, because if the Windows version of GdipIsMatrixEqual *is* catering for error of a given epsilon, then you can use that function for validation. If not, you have tests that prove that Windows is using a simplistic comparison method. 2. GdipIsMatrixInvertible which contains a check for 'not above zero' determinant. It has been a while since I have done any matrix math. I'll assume that the above is the correct definition/check for invertible (makes sense because you have inv(a(ij)) = a(ij)/det(A), so can't have det(A) == 0). Now here, you have several 'classes' of matrices w.r.t. whether they are invertible or not: a. det(A) 0 b. det(A) == 0 c. det(A) 0 So does Windows match your assumption for the answer to these (a == yes; b, c == no). I think first of all we should choose an appropriate method to do so and only after that try to compare results with native. What do you think of that? Are you saying that GdipIsMatrixInvertible should be used for comparison of two matrices? If so, you can have two different matrices that are invertible, so it does not make sense. A good first step would be to print out the expected and returned values for the matrices, and keep the GdipIsMatrixEqual check. This will allow you to determine if the reason this is failing is due to rounding errors, or that they are completely different results. - Reece
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Michael Karcher wrote: Am Sonntag, den 13.07.2008, 13:51 +0400 schrieb Nikolay Sivov: Reece Dunn wrote: Is this the way it works on Windows? You have 4 basic cases: a. exactly equal; b. equal + epsilon for rounding/representation errors; c. equal - epsilon for rounding/representation errors; d. not equal. I don't see a difference between b and c. A matrix has four elements. Some might be slightly too big and others slightly too small. Would this make b or c? It's just one case: equal +/- epsilon. Exactly, so I plan to test this deeper on native. As I see now after some basic tests, most probably native call does a bitwise comparison cause the result is affected by a smallest matrix element change (e.g. 1.000f - 1.001f). But I'll test it more. 1+FLT_EPSILON is the value to use for the smallest matrix element change. But sour test already is a very strong indication that it really is strict equality test. FLT_EPSILON is in float.h This header is optional, isn't it? I've used FLT_EPSILON in call PlgBlt of gdi32, but it was replaced with 1e-5 on commit (it was fabs(det) FLT_EPSILON). So am I right that we can't use it on portability reason? Reece, do you have any suggestions to do some bounds test (I mean test is call affected by representation or round errors or not)? Another thing you could test is: Is the Matrix (1,FLT_EPSILON/2,0,1) equal to (1,0,0,1). A matrix like te former can easily be the result of multiplying a matrix by its inverse matrix (if represented as floating point numbers). One might expect that the product of a matrix and its inverse always is the identity matrix. I'll try that. 2. GdipIsMatrixInvertible which contains a check for 'not above zero' determinant. a. det(A) 0 b. det(A) == 0 c. det(A) 0 So does Windows match your assumption for the answer to these (a == yes; b, c == no). What do you mean here by (c = no)? Native allows negative det and I do so. He implied it form your not above zero quote. Yes, I meant absolute value of course. A hint how I would implement that test (if it turns out that a strict comparison to 0 is not the right result): Do not calculate the sum of a*d and -b*c, but compare these numbers in a sensible way. Here I something like FLT_EPSILON constant. So it's the same like above. Regards, Michael Karcher
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
Am Sonntag, den 13.07.2008, 18:43 +0400 schrieb Nikolay Sivov:
Exactly, so I plan to test this deeper on native. As I see now after
some basic tests, most probably native call does a bitwise comparison
cause the result is affected by a smallest matrix element change (e.g.
1.000f - 1.001f). But I'll test it more.
1+FLT_EPSILON is the value to use for the smallest matrix element
change. But sour test already is a very strong indication that it
really is strict equality test. FLT_EPSILON is in float.h
This header is optional, isn't it? I've used FLT_EPSILON in call PlgBlt
of gdi32, but it was replaced with 1e-5 on commit (it was fabs(det)
FLT_EPSILON).
To the best of my knowledge and the web pages I found on this topic,
float.h is standard ANSI C. There should be no need to avoid this
header.
On the other hand, I don't understand why you use 1e-5 or FLT_EPSILON at
that point at all. If I am not totally confused, you calculate some kind
of determinant from the nRect entries, which were directly before
initialized from the nXSrc, nYSrc, nWidth, nHeight variables, which are
integers, so rect contains integer values. The formula for det only
contains subtraction and multiplication. How do you expect that the
outcome of this calculation is non-integral?
A hint how I would
implement that test (if it turns out that a strict comparison to 0 is
not the right result): Do not calculate the sum of a*d and -b*c, but
compare these numbers in a sensible way.
Here I something like FLT_EPSILON constant. So it's the same like above.
Be aware that 2+FLT_EPSILON stored into a float still is 2! You do not
want to write fabs(a*d-b*c) FLT_EPSILON.
Try this:
#include float.h
#include stdio.h
float det(float a, float b, float c, float d)
{
return fabs(a*b-c*d);
}
int main(void)
{
printf(%g %g\n,det(100./3,120./7,12000./21,1), FLT_EPSILON);
}
compiled with no optimization (to avoid the compiler pre-calculating the
determinant with higher precision). You will see that the difference is
*much* bigger than FLT_EPSILON, yet the matrix was intended to be
singular (and were singular if there were no rounding errors on the
thirds). You should understand the cause of this before you try to write
a robust float comparison function.
Regards,
Michael Karcher
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov [EMAIL PROTECTED] wrote:
Changelog:
- Fix GdipPathIterNextMarker behaviour on path without markers. Make
tests pass on native.
---
dlls/gdiplus/pathiterator.c |3 ++-
dlls/gdiplus/tests/pathiterator.c |6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
index 55b0782..3d3b1dc 100644
--- a/dlls/gdiplus/pathiterator.c
+++ b/dlls/gdiplus/pathiterator.c
@@ -140,7 +140,8 @@ GpStatus WINGDIPAPI
GdipPathIterNextMarker(GpPathIterator* iterator, INT *result
/* first call could start with second point as all subsequent, cause
path couldn't contain only one */
for(i = iterator-marker_pos + 1; i iterator-pathdata.Count; i++){
-if(iterator-pathdata.Types[i] PathPointTypePathMarker){
+if((iterator-pathdata.Types[i] PathPointTypePathMarker) ||
+ (i == iterator-pathdata.Count - 1)){
*startIndex = iterator-marker_pos;
if(iterator-marker_pos 0) (*startIndex)++;
*endIndex = iterator-marker_pos = i;
diff --git a/dlls/gdiplus/tests/pathiterator.c
b/dlls/gdiplus/tests/pathiterator.c
index 071c1d5..6498bb3 100644
--- a/dlls/gdiplus/tests/pathiterator.c
+++ b/dlls/gdiplus/tests/pathiterator.c
@@ -114,7 +114,7 @@ static void test_nextmarker(void)
GdipCreatePathIter(iter, path);
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(TRUE, (result == 4) (start == 0) (end == 3));
Why are you checking if stat == Ok? You're linking what should be
separate tests together. You also need to put each of these checks
into separate tests. If the test fails, you have no idea (without
debugging further) which one of those checks fails. They're called
unit tests for a reason.
GdipDeletePathIter(iter);
/* one marker */
@@ -125,7 +125,7 @@ static void test_nextmarker(void)
expect(TRUE, (start == 0) (end == 3) (result == 4));
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(0, result);
Same thing as above.
GdipDeletePathIter(iter);
/* two markers */
@@ -140,7 +140,7 @@ static void test_nextmarker(void)
expect(TRUE, (start == 4) (end == 5) (result == 2));
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(0, result);
Same.
--
James Hawkins
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
James Hawkins wrote:
On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov [EMAIL PROTECTED] wrote:
Changelog:
- Fix GdipPathIterNextMarker behaviour on path without markers. Make
tests pass on native.
---
dlls/gdiplus/pathiterator.c |3 ++-
dlls/gdiplus/tests/pathiterator.c |6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
index 55b0782..3d3b1dc 100644
--- a/dlls/gdiplus/pathiterator.c
+++ b/dlls/gdiplus/pathiterator.c
@@ -140,7 +140,8 @@ GpStatus WINGDIPAPI
GdipPathIterNextMarker(GpPathIterator* iterator, INT *result
/* first call could start with second point as all subsequent, cause
path couldn't contain only one */
for(i = iterator-marker_pos + 1; i iterator-pathdata.Count; i++){
-if(iterator-pathdata.Types[i] PathPointTypePathMarker){
+if((iterator-pathdata.Types[i] PathPointTypePathMarker) ||
+ (i == iterator-pathdata.Count - 1)){
*startIndex = iterator-marker_pos;
if(iterator-marker_pos 0) (*startIndex)++;
*endIndex = iterator-marker_pos = i;
diff --git a/dlls/gdiplus/tests/pathiterator.c
b/dlls/gdiplus/tests/pathiterator.c
index 071c1d5..6498bb3 100644
--- a/dlls/gdiplus/tests/pathiterator.c
+++ b/dlls/gdiplus/tests/pathiterator.c
@@ -114,7 +114,7 @@ static void test_nextmarker(void)
GdipCreatePathIter(iter, path);
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(TRUE, (result == 4) (start == 0) (end ==
3));
Why are you checking if stat == Ok? You're linking what should be
separate tests together. You also need to put each of these checks
into separate tests. If the test fails, you have no idea (without
debugging further) which one of those checks fails. They're called
unit tests for a reason.
I don't think so. If the call fails testing return value doesn't make
any sense. In this case 'result' is uninitialized and remains
uninitialized after a call if it fails (here I mean a native too). So
why should we check something which has unpredictable value?
By the way first time I saw this here
8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c..
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
On Sat, Jul 12, 2008 at 12:37 PM, Nikolay Sivov [EMAIL PROTECTED] wrote:
James Hawkins wrote:
On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov [EMAIL PROTECTED]
wrote:
Changelog:
- Fix GdipPathIterNextMarker behaviour on path without markers. Make
tests pass on native.
---
dlls/gdiplus/pathiterator.c |3 ++-
dlls/gdiplus/tests/pathiterator.c |6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
index 55b0782..3d3b1dc 100644
--- a/dlls/gdiplus/pathiterator.c
+++ b/dlls/gdiplus/pathiterator.c
@@ -140,7 +140,8 @@ GpStatus WINGDIPAPI
GdipPathIterNextMarker(GpPathIterator* iterator, INT *result
/* first call could start with second point as all subsequent, cause
path couldn't contain only one */
for(i = iterator-marker_pos + 1; i iterator-pathdata.Count; i++){
-if(iterator-pathdata.Types[i] PathPointTypePathMarker){
+if((iterator-pathdata.Types[i] PathPointTypePathMarker) ||
+ (i == iterator-pathdata.Count - 1)){
*startIndex = iterator-marker_pos;
if(iterator-marker_pos 0) (*startIndex)++;
*endIndex = iterator-marker_pos = i;
diff --git a/dlls/gdiplus/tests/pathiterator.c
b/dlls/gdiplus/tests/pathiterator.c
index 071c1d5..6498bb3 100644
--- a/dlls/gdiplus/tests/pathiterator.c
+++ b/dlls/gdiplus/tests/pathiterator.c
@@ -114,7 +114,7 @@ static void test_nextmarker(void)
GdipCreatePathIter(iter, path);
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(TRUE, (result == 4) (start == 0) (end ==
3));
Why are you checking if stat == Ok? You're linking what should be
separate tests together. You also need to put each of these checks
into separate tests. If the test fails, you have no idea (without
debugging further) which one of those checks fails. They're called
unit tests for a reason.
I don't think so. If the call fails testing return value doesn't make any
sense. In this case 'result' is uninitialized and remains uninitialized
after a call if it fails (here I mean a native too). So why should we check
something which has unpredictable value?
By the way first time I saw this here
8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c..
An API defines a unique output* for the set of all possible inputs.
When you write unit tests, you verify that the implementation of the
API conforms to the definition. In the case of Wine, our definition
of the Win32 API is the test results from running the Wine test suite
in Windows. While msdn is a good reference, it's not always correct
and we can't test our implementation against it.
In all the cases where you've added checks for (stat != Ok), stat does
equal Ok in Windows. Thus, the definition of the API for those inputs
is that the functions return Ok. If, for a hypothetical example, one
version of gdiplus did not return Ok, we would probably mark that as
broken. That leads me to my next point.
You say you added the checks because it's wrong to check the value of
an uninitialized variable. You are correct in that it is wrong to
check the value of an uninitialized variable. Where you are mistaken
is that, by the definition of this API, none of these out variables
will be uninitialized in these cases. One area where your tests are
lacking is that you don't control all of the input. Even in the
failure case, you should initialize all out variables to some magic
value and then check the values of those parameters after the failed
call. Either the values were unchanged, and thus still hold the magic
value, or they are set to some other value. One bonus of this
practice is that, even if the test fails in Wine when it should
succeed, you're guaranteed to never read the value of an uninitialized
variable.
So to answer your question, there should be no unpredictable values in
a well-written test.
* or a unique set of acceptable outputs, e.g., multiple GetLastError values.
--
James Hawkins
Re: [2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.
James Hawkins wrote:
On Sat, Jul 12, 2008 at 12:37 PM, Nikolay Sivov [EMAIL PROTECTED] wrote:
James Hawkins wrote:
On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov [EMAIL PROTECTED]
wrote:
Changelog:
- Fix GdipPathIterNextMarker behaviour on path without markers. Make
tests pass on native.
---
dlls/gdiplus/pathiterator.c |3 ++-
dlls/gdiplus/tests/pathiterator.c |6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
index 55b0782..3d3b1dc 100644
--- a/dlls/gdiplus/pathiterator.c
+++ b/dlls/gdiplus/pathiterator.c
@@ -140,7 +140,8 @@ GpStatus WINGDIPAPI
GdipPathIterNextMarker(GpPathIterator* iterator, INT *result
/* first call could start with second point as all subsequent, cause
path couldn't contain only one */
for(i = iterator-marker_pos + 1; i iterator-pathdata.Count; i++){
-if(iterator-pathdata.Types[i] PathPointTypePathMarker){
+if((iterator-pathdata.Types[i] PathPointTypePathMarker) ||
+ (i == iterator-pathdata.Count - 1)){
*startIndex = iterator-marker_pos;
if(iterator-marker_pos 0) (*startIndex)++;
*endIndex = iterator-marker_pos = i;
diff --git a/dlls/gdiplus/tests/pathiterator.c
b/dlls/gdiplus/tests/pathiterator.c
index 071c1d5..6498bb3 100644
--- a/dlls/gdiplus/tests/pathiterator.c
+++ b/dlls/gdiplus/tests/pathiterator.c
@@ -114,7 +114,7 @@ static void test_nextmarker(void)
GdipCreatePathIter(iter, path);
stat = GdipPathIterNextMarker(iter, result, start, end);
expect(Ok, stat);
-expect(0, result);
+if(stat == Ok) expect(TRUE, (result == 4) (start == 0) (end ==
3));
Why are you checking if stat == Ok? You're linking what should be
separate tests together. You also need to put each of these checks
into separate tests. If the test fails, you have no idea (without
debugging further) which one of those checks fails. They're called
unit tests for a reason.
I don't think so. If the call fails testing return value doesn't make any
sense. In this case 'result' is uninitialized and remains uninitialized
after a call if it fails (here I mean a native too). So why should we check
something which has unpredictable value?
By the way first time I saw this here
8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c..
An API defines a unique output* for the set of all possible inputs.
When you write unit tests, you verify that the implementation of the
API conforms to the definition. In the case of Wine, our definition
of the Win32 API is the test results from running the Wine test suite
in Windows. While msdn is a good reference, it's not always correct
and we can't test our implementation against it.
If you mean that an API's defined by a mapping of all possible inputs to
a set of outputs I'm agreed.
In all the cases where you've added checks for (stat != Ok), stat does
equal Ok in Windows. Thus, the definition of the API for those inputs
is that the functions return Ok. If, for a hypothetical example, one
version of gdiplus did not return Ok, we would probably mark that as
broken. That leads me to my next point.
You say you added the checks because it's wrong to check the value of
an uninitialized variable. You are correct in that it is wrong to
check the value of an uninitialized variable. Where you are mistaken
is that, by the definition of this API, none of these out variables
will be uninitialized in these cases. One area where your tests are
lacking is that you don't control all of the input. Even in the
failure case, you should initialize all out variables to some magic
value and then check the values of those parameters after the failed
call. Either the values were unchanged, and thus still hold the magic
value, or they are set to some other value. One bonus of this
practice is that, even if the test fails in Wine when it should
succeed, you're guaranteed to never read the value of an uninitialized
variable.
So you mean here that in this case testing return value should be
unlinked to testing outputs, that these tests are independent. Maybe it
really helps when behaviour changes (API changes in fact) from version
to version. So maybe it's the better way to split this and let it both
test fail.
So to answer your question, there should be no unpredictable values in
a well-written test.
* or a unique set of acceptable outputs, e.g., multiple GetLastError values.
Yes, I thought about some special 'magic' values for output arguments.
Some deadbeef should be acceptable.
Thanks for comments I'll repost these fixes.
