On 07/25/2010 01:55 PM, James McKenzie wrote:
Andrew Eikum wrote:
On 07/25/2010 12:04 PM, Max TenEyck Woodbury wrote:
On 07/25/2010 09:45 AM, James McKenzie wrote:
I think you missed what Nicolay and Dmitry are trying to tell you.
We are trying to implement, bug for bug, the functionality of what
Windows does. Does Windows return "STATUS_NOT_IMPLEMENTED" when this
call is made? If not, your fix is WRONG. Silencing a 'fixme' is NOT a
fix and this will be REJECTED.
If this is correct and is what Windows does, then state so. Otherwise,
withdraw the patch and fix it the right way.
James McKenzie
Frankly, I do not know what Microsoft does, but the test would fail on
their implementation if they did something else, so I think it is safe
to assume the test is implemented properly. Given that, the fixme is
wrong.
You had very well much know what Microsoft does and care very much about
what they do. The goal of this project, as it has been since the mid
1990s is to fully emulate, bug and all, the Microsoft Windows32 and
Windows64 (since 64 bit versions of Windows arrived) APIs. Thus we have
test cases that demonstrate what the actions are of the API/ABI. That is
what I've been working on with several richedit functions that I need to
have for programs that I personally use. I'm 'eating my own dog food' to
speak.
Since I do not have (and should never have if I work on Wine) access to
Microsoft's code, I can not know what they actually do.
The test case code is what is available. I did not write this test case
code, but it has been run against Microsoft's code and has been around
long enough that it would have been corrected if it did not match their
results. I believe it is also safe to assume that the Microsoft code
does NOT produce 'fixme' messages. Unless the test case code is wrong,
there should not be a 'fixme' produced.
Specifically, Nicolay asked for a test case. Since I was working from
an already existing test case, his request didn't really make sense. I
pointed out that there already was a test case and that should have
been the end of it.
Well, you didn't point out which existing testcase you are talking
about. All that your patch does is silence a FIXME. Presumably, the
FIXME was placed there for a reason. Nikolay and Dmitry were pointing
out that silencing that FIXME might not be appropriate, and were
asking for you to demonstrate why that FIXME is invalid. Adding a
testcase or pointing to an existing testcase would accomplish this.
I looked at the testcase. He will need to 'beef up' the testcase to
demonstrate what functions that particular test case does. If it does
return "not implemented" then it has to be demonstrated. I tried to
point this out as did Nicolay and Dmitry. Otherwise, this patch will be
rejected by AJ as an attempt to silence the fixme, which he detests
greatly.
You obviously found the test case code. I did not write it. If it
reports failures when run against Microsoft's code, it needs fixing,
but I am not the one to fix it. As it stands, the expected result is a
'not implemented' status. Given that, any 'fixme' issued would be
improper.
Which existing testcase demonstrates that this behavior is valid and
that the FIXME is unwarranted? Does the existing testcase demonstrate
the full range of behavior given that parameter? Can you expand on the
tests to show that your implementation is always correct?
Now you come along and make loud demands that the patch be rejected,
without having looked at the situation carefully. Frankly, this looks
very much like the activities of an 'in-crowd' trying to defend its
boarders.
No. we have stated that you have no authoritative source that you are
correct. State this and we all will be satisfied that the patch you
propose is correct. Right now, it appears as if the patch is just a
'silence the fixme' attempt. This is why your original set of patches
was beat up. We want to know, and so do our users, that implementation
is not complete for functions. If this is breaking a program you are
using, you are welcome to fully implement the function. That is what we
are all here for. Fixmes are not the way to go, but if a stub makes
things work that is the first step of many. There are many fixmes in
Wine code and I'm working on three of them in the richedit dll that
directly affect the ability for me to properly use programs. I've built
a test case for one of them, that was rejected by AJ. I'm working on a
second test case and then will build out code from that position. This
is how the project moves ahead.
There is no 'authoritative source'. There are only the test cases. The
'fixme' appears to be the result of slightly sloppy coding. Since the
test case shows that the proper behavior is to return an error code,
the 'fixme' is an error. The patch is intended to bring Wine's behavior
into line with that of the Microsoft code. It is not "just a 'silence
the fixme' attempt." This particular 'fixme' should never have been
issued.
As for the '_ONCE' patches, they were withdrawn because of technical
problems. I did note the objections to limiting stub reports to 'one
time only' and did cut back to only the extant limits before
withdrawing the whole set, so please get your facts straight.
You need to take it easy, man. No one is out to get you :)
We really are not out to get you. I work, daily, as a software QA
analyst. I reject perfectly good looking software because it behaves
strangely and does not react properly to improper inputs. It is not easy
being on that end of the process, however I also understand that writing
proper code and having good/superb test cases makes all the difference.
The bottom line is that there is no test case that validates your
change. Create one. If your change is wrong, you then have the knowledge
to state "I withdraw". I've done that, several times. If it is right,
then add the test case, first, then the new code and state this is based
on your test case and when you ran it in the change log. Makes us all
happy and it furthers the state of Wine.
Here you are flat out wrong. There is a test case. I did not write it;
someone else did. It does not need to be duplicated. It shows that the
'fixme' is not appropriate. Specifically, the test is at
dlls/ntdll/tests/file.c:1110-1111.
Wine has a high barrier for entry and patches are reviewed harshly. If
people are responding negatively to your patch, then it's likely
because your patch was not obviously correct. The correct way to
respond to this is by proving that it's correct, not asserting that
it's correct. You're going to have to deal with defending your
patches, and accept that sometimes you are wrong and sometimes other
people are wrong.
Also, strongly defending your patches without authoritative information
marks you as being arrogant. After a while, your patches will be
ignored. That is not a good place to be in this project.
James
I have cited an existing test case. If that is not authoritative
enough, please explain what is actually required.
- Max
