Changelog:
Re-implement all *printf functions in msvcrt.dll, according to http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/...
and descriptions of appropriate functions. This should fix, among other things, bugs 321 and 2075.
Nobody has any comments about this?
On Fri, 28 Jan 2005 12:48:12 +0000, Aneurin Price alp108@cs.york.ac.uk wrote:
Changelog:
Re-implement all *printf functions in msvcrt.dll, according to http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/...
and descriptions of appropriate functions. This should fix, among other things, bugs 321 and 2075.
Nobody has any comments about this?
Sorry for being late at responding. I took a look at it finally like a week ago. You can definately break up the patch. I'd say make the moving of the functions from file.c into printf.c a seperate patch, the last one really. It's extra noise right now.
You could also handle the wide-char printf changes seperate as well as changes to the fprintfish functions. Maybe if you do this, you can migrate printf in steps. wide-char printf and fprintf's already work for what they do, so we can impliment the standard printf first, test it out itself and then moving in the other patches should be dead simple. I'm not sure 100% on this, because... I've never done this before! But since I've been studying this for some time I think I can least make the suggestion =).
Jesse
On Fri, 28 Jan 2005 18:18:40 -0700, Jesse Allen the3dfxdude@gmail.com wrote:
On Fri, 28 Jan 2005 12:48:12 +0000, Aneurin Price
Nobody has any comments about this?
Sorry for being late at responding. I took a look at it finally like a week ago. You can definately break up the patch.
Erm, nevermind on breaking it up yet. I started to myself, but I think it's more trouble than it's worth right now. We can have that done later if that is what is wanted to have it accepted.
I had another idea to help testing. I added a comparison test in _vsnprintf to go ahead and see if there is any difference between libc and wine by calling libc's version. If there is a difference, it will print a warning. Here's an obvious example in action while running War3:
warn:msvcrt:_vsnprintf -- vsnprintf's differ -- warn:msvcrt:_vsnprintf Wine (20): "-4035225266123964415" warn:msvcrt:_vsnprintf libc (64): " 1" warn:msvcrt:_vsnprintf format string: "%I64d"
If we took a look at this, we would find that there is no bug in the difference, and actually the improvement over libc's version that we are expecting. So this warning can be discarded.
If we found another warning, the difference may definately be a bug, and then it can be fixed when analyzed. We could also add a conformance test to make sure it is fixed and doesn't happen again.
Of course this method doesn't help with crashing bugs. And it makes vsnprintf *2 slower, but it doesn't matter when we are testing it.
I have a diff below which is against wine CVS + your printf patch.
Jesse
--- wine/dlls/msvcrt/printf.c 2005-01-29 11:21:54.000000000 -0700 +++ wine-printf/dlls/msvcrt/printf.c 2005-01-29 11:07:23.000000000 -0700 @@ -1134,6 +1134,8 @@ */ int _vsnprintf(char *buffer, size_t count, const char *format, va_list argptr) { + char *compare; + va_list argptrcpy = argptr; /* Since the meaning of type specifiers %S and %s (and C/c) invert * depending on whether the function called is *wprintf or *printf, * this doesn't simply convert to wide, call _vsnwprintf, and @@ -1196,6 +1198,19 @@ WideCharToMultiByte(CP_ACP, 0, out, -1, buffer, count + 1, NULL, NULL); HeapFree(GetProcessHeap(), 0, out); HeapFree(GetProcessHeap(), 0, in); + + compare = HeapAlloc(GetProcessHeap(),0,count+1); + if (compare != NULL) { + int comparePrinted = vsnprintf(compare, count, format, argptrcpy); + if (strncmp(compare, buffer, count)!=0) { + WARN("-- vsnprintf's differ --\n"); + WARN("Wine (%d): "%s"\n", charsPrinted, buffer); + WARN("libc (%d): "%s"\n", comparePrinted, compare); + WARN("format string: "%s"\n", format); + } + HeapFree(GetProcessHeap(), 0, compare); + } + return charsPrinted; }
Jesse Allen wrote:
Sorry for being late at responding. I took a look at it finally like a week ago. You can definately break up the patch.
Erm, nevermind on breaking it up yet. I started to myself, but I think it's more trouble than it's worth right now. We can have that done later if that is what is wanted to have it accepted.
I had another idea to help testing. I added a comparison test in _vsnprintf to go ahead and see if there is any difference between libc and wine by calling libc's version. If there is a difference, it will print a warning. Here's an obvious example in action while running War3:
<snip> Cool, that's a good idea. I was holding off on responding to the other 2 posts until I'd come up with a meaningful response, but it's probably better to acknowledge them :). In response to Juan's comments then: 1. Already done :). I had several test cases (including one for the example you give) committed some time ago. 2. Well I can certainly give it some thought, though I'm not sure yet how I'd want to do it. This is the part that made me want to wait before responding, since I don't have anything actually useful to add yet. Anyway, I'm not ignoring you, and will respond decently when I've found the time to play with this a bit :). Thanks for the input.
-
Aneurin Price -
Aneurin Price -
Jesse Allen