Re: new initdb.c available

Thanks. I will attend to most of this. A couple of points:

. using "wb" for writing out on Windows is so that we don't get Windows' 
gratuitous addition of carriage returns. I will document that.
. I can't use do { stuff; } while(0) for a macro that does declarations 
- the declarations would be local to the do block.

Doesn't pg_indent do the spacing for us when code is merged? I guess I 
can get BSD indent - my Linux box only has GNU indent. If indent won't 
do spacing I'll go through and do it by hand.

Expect a new version in a few days - with the removal of the 
initdb-scripts.h as well as these changes.

cheers

andrew




Peter Eisentraut wrote:

>Andrew Dunstan writes:
>
>  
>
>>New version has passed basic Windows tests, and is available (with new
>>Makefile too) at http://www.dunslane.net/~andrew/Initdb_In_C.html
>>
>>constructive comments (very) welcome.
>>    
>>
>
>That looks very nice.  Just some nitpicking comments.  (Most of these
>comments should be extrapolated to similar source code fragments.)
>
>
>  
>
>>#include <getopt_long.h>
>>    
>>
>
>Must be "getopt_long.h" because it might be our own replacement file.
>
>  
>
>>#include <sys/types.h>
>>    
>>
>
>Already done in c.h.
>
>  
>
>>/* who we are */
>>#define PG_VERSIONSTR "postgres (PostgreSQL) " PG_VERSION "\n"
>>    
>>
>
>Should be "initdb (PostgreSQL) ...".
>
>  
>
>>#define WRITEMODE "wb"
>>    
>>
>
>Use #define PG_BINARY_W "wb", if you are writing "binary" files, which
>isn't quite clear.
>
>  
>
>>/*
>> * macros for running pipes to postgres
>> * note lack of trailing ';' must be placed there when macros are used.
>> * this keeps emacs indentation happy
>> */
>>
>>#define PG_CMD_DECL     char cmd[MAXPGPATH]; char ** line ; FILE * pg
>>    
>>
>
>Use
>
>#define MACRO do { code; here; } while(0)
>
>That's the standard mechanism.
>
>  
>
>>#endif
>>    
>>
>
>Write "#endif /* WIN32 */", or whatever the case may be.
>
>  
>
>>#define malloc(x) xmalloc( (x) )
>>    
>>
>
>You are not allowed to redefine or otherwise fiddle with standard library
>functions.  Just write xmalloc when you mean xmalloc.
>
>  
>
>>if (strcmp(file->d_name,".") && strcmp(file->d_name,".."))
>>    
>>
>
>Please compare the result of strcmp() with 0.  Code like this makes my
>brain hurt.  Do
>
>#define streq(a, b) (strcmp((a), (b))==0)
>
>if you must.
>
>  
>
>>snprintf(filepath,MAXPGPATH,"%s/%s",path,*filename);
>>    
>>
>
>Spaces after the commas.  Spaces after semicolons.  Spaces before and
>after binary operators.  More spaces everywhere.
>
>  
>
>>static char *
>>pg_getlocale(char * arg)
>>    
>>
>
>This is redundant.  In C you can just use, for example,
>
>locale = setlocale(LC_CTYPE, NULL);
>
>This is actually one of the nice things we'll get out of having this in C.
>
>  
>
>>sizeof(char)
>>    
>>
>
>... is always 1.
>
>  
>
>>newtext = replace_token(usage_text,"$CMDNAME",progname);
>>
>>for (i=0; newtext[i]; i++)
>>    fputs(newtext[i],stdout);
>>    
>>
>
>Uh, why not use printf directly?
>
>  
>
>>if (show_version)
>>{
>>    printf("%s (PostgreSQL) %s\n",argv[0],PG_VERSION);
>>    exit(0);
>>}
>>    
>>
>
>For the --version output, the program name is always hardcoded, to allow
>identification in case the program was renamed.
>
>  
>
>>       if (!mkdatadir(NULL))
>>       {
>>           exit_nicely();
>>       }
>>       else
>>           check_ok();
>>    
>>
>
>Bizarre use of braces.
>
>  
>