Any further comments on this?
Aneurin Price wine@shadovald.dyndns.org writes:
Any further comments on this?
Hard to say, that code is really painful to read. Please try to better follow the coding conventions used in the rest of the code.
Alexandre Julliard wrote:
Aneurin Price wine@shadovald.dyndns.org writes:
Any further comments on this?
Hard to say, that code is really painful to read. Please try to better follow the coding conventions used in the rest of the code.
Apologies for long delay; I've been playing World of Warcraft for the duration of the open beta :-). I don't really understand what it is you want, but I think I'm going to have to give up on this for the moment, until I get enough time to get more coding practice. Aneurin Price
On Thu, Nov 18, 2004 at 04:16:02PM +0000, Aneurin Price wrote:
I don't really understand what it is you want, but I think I'm going to have to give up on this for the moment, until I get enough time to get more coding practice. Aneurin Price
If you don't mind, I will take up hacking it. Like I said, I started my own printf a while back. I'm not sure what path I would take with your code. I'll have to study it thoroughly. I may just improve on it, or I may use ideas from it to make mine functional. My last version may be worth reviving, because it tried to mimick wine style coding as much as it could. If I were to use your code, I have some reproducible issues that need to be solved to start. But I need to see how well I can understand your code. So lets see what happens this next week.
Jesse
Well I recently got re-motivated, by forgetting I was using an unpatched version of Wine, and getting my WC3 campaign file nuked :(.
So, I've had a go over this all, and here is a new version. Is it a step in the right direction? I still don't really know what I should be doing to improve this. Aneurin Price
Index: dlls/msvcrt/Makefile.in =================================================================== RCS file: /home/wine/wine/dlls/msvcrt/Makefile.in,v retrieving revision 1.17 diff -u -u -r1.17 Makefile.in --- dlls/msvcrt/Makefile.in 8 Nov 2004 22:10:43 -0000 1.17 +++ dlls/msvcrt/Makefile.in 27 Nov 2004 21:27:58 -0000 @@ -27,6 +27,7 @@ math.c \ mbcs.c \ misc.c \ + printf.c \ process.c \ scanf.c \ string.c \ Index: dlls/msvcrt/file.c =================================================================== RCS file: /home/wine/wine/dlls/msvcrt/file.c,v retrieving revision 1.73 diff -u -u -r1.73 file.c --- dlls/msvcrt/file.c 3 Nov 2004 22:17:05 -0000 1.73 +++ dlls/msvcrt/file.c 27 Nov 2004 21:28:00 -0000 @@ -2614,113 +2614,6 @@ return file; }
-/********************************************************************* - * vfprintf (MSVCRT.@) - */ -int MSVCRT_vfprintf(MSVCRT_FILE* file, const char *format, va_list valist) -{ - char buf[2048], *mem = buf; - int written, resize = sizeof(buf), retval; - /* There are two conventions for vsnprintf failing: - * Return -1 if we truncated, or - * Return the number of bytes that would have been written - * The code below handles both cases - */ - while ((written = vsnprintf(mem, resize, format, valist)) == -1 || - written > resize) - { - resize = (written == -1 ? resize * 2 : written + 1); - if (mem != buf) - MSVCRT_free (mem); - if (!(mem = (char *)MSVCRT_malloc(resize))) - return MSVCRT_EOF; - } - retval = MSVCRT_fwrite(mem, sizeof(*mem), written, file); - if (mem != buf) - MSVCRT_free (mem); - return retval; -} - -/********************************************************************* - * vfwprintf (MSVCRT.@) - * FIXME: - * Is final char included in written (then resize is too big) or not - * (then we must test for equality too)? - */ -int MSVCRT_vfwprintf(MSVCRT_FILE* file, const MSVCRT_wchar_t *format, va_list valist) -{ - MSVCRT_wchar_t buf[2048], *mem = buf; - int written, resize = sizeof(buf) / sizeof(MSVCRT_wchar_t), retval; - /* See vfprintf comments */ - while ((written = _vsnwprintf(mem, resize, format, valist)) == -1 || - written > resize) - { - resize = (written == -1 ? resize * 2 : written + sizeof(MSVCRT_wchar_t)); - if (mem != buf) - MSVCRT_free (mem); - if (!(mem = (MSVCRT_wchar_t *)MSVCRT_malloc(resize*sizeof(*mem)))) - return MSVCRT_EOF; - } - retval = MSVCRT_fwrite(mem, sizeof(*mem), written, file); - if (mem != buf) - MSVCRT_free (mem); - return retval; -} - -/********************************************************************* - * vprintf (MSVCRT.@) - */ -int MSVCRT_vprintf(const char *format, va_list valist) -{ - return MSVCRT_vfprintf(MSVCRT_stdout,format,valist); -} - -/********************************************************************* - * vwprintf (MSVCRT.@) - */ -int MSVCRT_vwprintf(const MSVCRT_wchar_t *format, va_list valist) -{ - return MSVCRT_vfwprintf(MSVCRT_stdout,format,valist); -} - -/********************************************************************* - * fprintf (MSVCRT.@) - */ -int MSVCRT_fprintf(MSVCRT_FILE* file, const char *format, ...) -{ - va_list valist; - int res; - va_start(valist, format); - res = MSVCRT_vfprintf(file, format, valist); - va_end(valist); - return res; -} - -/********************************************************************* - * fwprintf (MSVCRT.@) - */ -int MSVCRT_fwprintf(MSVCRT_FILE* file, const MSVCRT_wchar_t *format, ...) -{ - va_list valist; - int res; - va_start(valist, format); - res = MSVCRT_vfwprintf(file, format, valist); - va_end(valist); - return res; -} - -/********************************************************************* - * printf (MSVCRT.@) - */ -int MSVCRT_printf(const char *format, ...) -{ - va_list valist; - int res; - va_start(valist, format); - res = MSVCRT_vfprintf(MSVCRT_stdout, format, valist); - va_end(valist); - return res; -}
/********************************************************************* * ungetc (MSVCRT.@) @@ -2753,17 +2646,4 @@ return MSVCRT_WEOF; } return mwc; -} - -/********************************************************************* - * wprintf (MSVCRT.@) - */ -int MSVCRT_wprintf(const MSVCRT_wchar_t *format, ...) -{ - va_list valist; - int res; - va_start(valist, format); - res = MSVCRT_vwprintf(format, valist); - va_end(valist); - return res; } Index: dlls/msvcrt/msvcrt.spec =================================================================== RCS file: /home/wine/wine/dlls/msvcrt/msvcrt.spec,v retrieving revision 1.93 diff -u -u -r1.93 msvcrt.spec --- dlls/msvcrt/msvcrt.spec 14 Oct 2004 00:26:39 -0000 1.93 +++ dlls/msvcrt/msvcrt.spec 27 Nov 2004 21:28:00 -0000 @@ -433,8 +433,8 @@ @ cdecl _setmode(long long) @ stub _setsystime #(ptr long) @ cdecl _sleep(long) -@ varargs _snprintf(str long str) snprintf -@ varargs _snwprintf(wstr long wstr) ntdll._snwprintf +@ varargs _snprintf(str long str) +@ varargs _snwprintf(wstr long wstr) @ varargs _sopen(str long long) MSVCRT__sopen @ varargs _spawnl(long str str) @ varargs _spawnle(long str str) @@ -484,7 +484,7 @@ @ cdecl _unloaddll(long) @ cdecl _unlock(long) @ cdecl _utime(str ptr) -@ cdecl _vsnprintf(ptr long ptr ptr) vsnprintf +@ cdecl _vsnprintf(ptr long ptr ptr) @ cdecl _vsnwprintf(ptr long wstr long) @ cdecl _waccess(wstr long) @ stub _wasctime #(ptr) MSVCRT__wasctime @@ -693,7 +693,7 @@ @ cdecl signal(long long) MSVCRT_signal @ cdecl sin(double) @ cdecl sinh(double) -@ varargs sprintf(ptr str) +@ varargs sprintf(ptr str) MSVCRT_sprintf @ cdecl sqrt(double) @ cdecl srand(long) @ varargs sscanf(str str) MSVCRT_sscanf @@ -735,7 +735,7 @@ @ cdecl vfprintf(ptr str long) MSVCRT_vfprintf @ cdecl vfwprintf(ptr wstr long) MSVCRT_vfwprintf @ cdecl vprintf(str long) MSVCRT_vprintf -@ cdecl vsprintf(ptr str ptr) +@ cdecl vsprintf(ptr str ptr) MSVCRT_vsprintf @ cdecl vswprintf(ptr wstr long) MSVCRT_vswprintf @ cdecl vwprintf(wstr long) MSVCRT_vwprintf @ cdecl wcscat(wstr wstr) ntdll.wcscat Index: dlls/msvcrt/wcs.c =================================================================== RCS file: /home/wine/wine/dlls/msvcrt/wcs.c,v retrieving revision 1.18 diff -u -u -r1.18 wcs.c --- dlls/msvcrt/wcs.c 25 Jun 2004 01:19:15 -0000 1.18 +++ dlls/msvcrt/wcs.c 27 Nov 2004 21:28:00 -0000 @@ -178,194 +178,6 @@ return ret; }
-static int MSVCRT_vsnprintfW(WCHAR *str, size_t len, const WCHAR *format, va_list valist) -{ - unsigned int written = 0; - const WCHAR *iter = format; - char bufa[256], fmtbufa[64], *fmta; - - while (*iter) - { - while (*iter && *iter != '%') - { - if (written++ >= len) - return -1; - *str++ = *iter++; - } - if (*iter == '%') - { - if (iter[1] == '%') - { - if (written++ >= len) - return -1; - *str++ = '%'; /* "%%"->'%' */ - iter += 2; - continue; - } - - fmta = fmtbufa; - *fmta++ = *iter++; - while (*iter == '0' || - *iter == '+' || - *iter == '-' || - *iter == ' ' || - *iter == '*' || - *iter == '#') - { - if (*iter == '*') - { - char *buffiter = bufa; - int fieldlen = va_arg(valist, int); - sprintf(buffiter, "%d", fieldlen); - while (*buffiter) - *fmta++ = *buffiter++; - } - else - *fmta++ = *iter; - iter++; - } - - while (isdigit(*iter)) - *fmta++ = *iter++; - - if (*iter == '.') - { - *fmta++ = *iter++; - if (*iter == '*') - { - char *buffiter = bufa; - int fieldlen = va_arg(valist, int); - sprintf(buffiter, "%d", fieldlen); - while (*buffiter) - *fmta++ = *buffiter++; - } - else - while (isdigit(*iter)) - *fmta++ = *iter++; - } - if (*iter == 'h' || *iter == 'l') - *fmta++ = *iter++; - - switch (*iter) - { - case 'S': - { - static const char *none = "(null)"; - const char *astr = va_arg(valist, const char *); - const char *striter = astr ? astr : none; - int r, n; - while (*striter) - { - if (written >= len) - return -1; - n = 1; - if( IsDBCSLeadByte( *striter ) ) - n++; - r = MultiByteToWideChar( CP_ACP, 0, - striter, n, str, len - written ); - striter += n; - str += r; - written += r; - } - iter++; - break; - } - - case 's': - { - static const WCHAR none[] = { '(','n','u','l','l',')',0 }; - const WCHAR *wstr = va_arg(valist, const WCHAR *); - const WCHAR *striter = wstr ? wstr : none; - while (*striter) - { - if (written++ >= len) - return -1; - *str++ = *striter++; - } - iter++; - break; - } - - case 'c': - if (written++ >= len) - return -1; - *str++ = (WCHAR)va_arg(valist, int); - iter++; - break; - - default: - { - /* For non wc types, use system sprintf and append to wide char output */ - /* FIXME: for unrecognised types, should ignore % when printing */ - char *bufaiter = bufa; - if (*iter == 'p') - sprintf(bufaiter, "%08lX", va_arg(valist, long)); - else - { - *fmta++ = *iter; - *fmta = '\0'; - if (*iter == 'a' || *iter == 'A' || - *iter == 'e' || *iter == 'E' || - *iter == 'f' || *iter == 'F' || - *iter == 'g' || *iter == 'G') - sprintf(bufaiter, fmtbufa, va_arg(valist, double)); - else - { - /* FIXME: On 32 bit systems this doesn't handle int 64's. - * on 64 bit systems this doesn't work for 32 bit types - */ - sprintf(bufaiter, fmtbufa, va_arg(valist, void *)); - } - } - while (*bufaiter) - { - if (written++ >= len) - return -1; - *str++ = *bufaiter++; - } - iter++; - break; - } - } - } - } - if (written >= len) - return -1; - *str++ = 0; - return (int)written; -} - -/********************************************************************* - * swprintf (MSVCRT.@) - */ -int MSVCRT_swprintf( MSVCRT_wchar_t *str, const MSVCRT_wchar_t *format, ... ) -{ - va_list ap; - int r; - - va_start( ap, format ); - r = MSVCRT_vsnprintfW( str, INT_MAX, format, ap ); - va_end( ap ); - return r; -} - -/********************************************************************* - * _vsnwprintf (MSVCRT.@) - */ -int _vsnwprintf(MSVCRT_wchar_t *str, unsigned int len, - const MSVCRT_wchar_t *format, va_list valist) -{ - return MSVCRT_vsnprintfW(str, len, format, valist); -} - -/********************************************************************* - * vswprintf (MSVCRT.@) - */ -int MSVCRT_vswprintf( MSVCRT_wchar_t* str, const MSVCRT_wchar_t* format, va_list args ) -{ - return MSVCRT_vsnprintfW( str, INT_MAX, format, args ); -} - /********************************************************************* * wcscoll (MSVCRT.@) */
On Sat, 27 Nov 2004 20:37:21 +0000, Aneurin Price wrote:
So, I've had a go over this all, and here is a new version. Is it a step in the right direction? I still don't really know what I should be doing to improve this.
Yep, that's a lot better. There is still room for improvement though:
1) You add new files by diffing against /dev/null:
diff -u /dev/null dlls/msvcrt/printf.c >>/tmp/patch
otherwise AJ can't apply the patch easily.
2) The code style needs a bit of work:
- There's not enough whitespace. Put spaces in between operators, ie instead of:
specifier.flags&=~SHORT_CHAR;
write
specifier.flags &= ~SHORT_CHAR;
and put blank lines in between variable declarations and code. Blank lines help split up large blocks of code and make them look easier to read.
- Comments like this:
/**Size: ** **/
are a bit hard to read. Just write /* size */, ie no doubling of asterisks, no extraneous lines, and not left aligned in the source.
- Function names like these:
gfloattostringw
are also hard to read, it'd be better as g_float_to_stringW. I understand that looks a bit odd, but it's the convention.
- I know this one isn't your fault but you may want to use this:
typedef mswchar MSVCRT_wchar_t;
to reduce visual noise.
- Please don't embed flag constants directly like this:
MSVCRT_wchar_t *input=(MSVCRT_wchar_t *)HeapAlloc(GetProcessHeap(),8,(1+strlen(format))*sizeof(MSVCRT_wchar_t));
eg what is 8 in this context? Use HEAP_ZERO_MEMORY or whatever.
- Feel free to wrap lines if they get too long
- Usually better to write this:
MSVCRT_wchar_t buf[2048] MSVCRT_whcar_T *mem = buf;
than this:
MSVCRT_wchar_t buf[2048], *mem = buf;
- Use the else keyword! Eg instead of this:
if (num>0) length=ceil(log10(num)); /**Need to determine the string length**/ if (num==0) /**necessary to hold the output. **/ length=1; if (num<0) length=ceil(log10(-num));
write this:
if (num > 0) /* need to determine the string length */ length = ceil(log10(num)); else if (num == 0) /* necessary to hold the output */ length = 1; else if (num < 0) length = ceil(log10(-num));
By the way, this seems a bit redundant: you can use abs() to ensure a number is always positive, eg:
if (num) { /* need to determine string length */ length = ceil(log10(abs(num))); } else { length = 1; }
- Multi-line comments shouldn't use a separate comment for each line:
while (num) { *tempstring++=digits[(num%10)]; /**Find each digit (starting at the little end)**/ num/=10; /**tempstring then points to the output string, backwards**/ charsdone++; /**Ie. it's used as a stack from which chars are popped **/ } /**and pushed on to the output. **/
could be:
while (num) { /* Find each digit (starting at the little end). * tempstring then points to the output string, backwards, * ie it's used as a stack from which chars are popped. */
*tempstring++ = digits[(num % 10)]; /* what's up with the superflous () here? */ num /= 10; charsdone++; }
- What is this ?
if ((specifier.precision!=-1) && (specifier.width>specifier.precision)) (specifier.flags&=~ZERO_PAD); /**If there is a precision specified, and it is less**/ /**than the specified width, don't use 0s to pad **/
This is exactly the sort of thing Alexandre meant when he said the patch was hard to read. This is an if statement that looks like it does nothing but actually does. It's also combined with one of those hard to read comments. Why is the statement enclosed in unnecessary brackets which make it look like a part of the expression? Write it like this instead:
/* If there is a precision specified, and it is less than the specified width, * don't use zeros to pad. */ if ((specifier.precision != -1) && (specifier.width > specifier.precision)) specifier.flags &= ~ZERO_PAD;
3) Why do you mix up MSVCRT_malloc, MSVCRT_free and HeapAlloc/HeapFree?
4) Watch out for // style comments. It's an easy mistake to make (i have done it before) but they aren't allowed.
5) #include "printfhelpers.c" <-- this sort of thing is very unusual. Why not use standard linkage, or include it all in one file? If the file is getting too big then don't just #include other C files as that's unexpected, split them up into separate compilation units.
6) Use a FIXME() especially in unusual situations that won't spam the user:
if (specifier.precision>=4) *(tempstring-4)='N'; /**FIXME:Is this actually the right string? MSDN **/ [snip]
Always do what the native DLL does, ignore MSDN if it's wrong. If you decide the miss bits out, that's OK but make sure it's obvious with a nice big FIXME("Not quite the same as native\n"); statement so people debugging can get a hint.
7) If you want to switch some code off (why is it being submitted if it's unnecessary?) use #if 0/#endif not multi-line comments like this:
/* if (specifier.flags&PRECISION_COUNT) { specifier.doubleIn=in; HeapFree(GetProcessHeap(),specifier.output); return floattostringw(args,digits,specifier); } */
as it's quite easy to miss them if you're not using a syntax-highlighting editor (a patch viewer for instance)
That's a lot of comments! Still, don't be discouraged, the code itself seems fairly sound, it's clearly been tested against native which is great, and it seems very complete which is also a good start. Good luck!
thanks -mike
Mike Hearn wrote:
On Sat, 27 Nov 2004 20:37:21 +0000, Aneurin Price wrote:
<big snip> Thanks for that; it's just the kind of comment I was looking for. It wasn't a full patch as I wasn't planning to get it applied, and a number of points were where I copied code from what was there already :), but you even caught a couple of places where I said to myself things like "I'll remeber to remove that commented block". I'll get to work on this today with any luck.
-
Alexandre Julliard -
Aneurin Price -
Jesse Allen -
Mike Hearn