Re: Resend: Re-implement MSVCRT *printf functions

[Mike Hearn]

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