Re: new initdb.c available - Mailing list pgsql-hackers
From | Andrew Dunstan |
---|---|
Subject | Re: new initdb.c available |
Date | |
Msg-id | 3F8202F1.4070304@dunslane.net Whole thread Raw |
In response to | Re: new initdb.c available (Peter Eisentraut <peter_e@gmx.net>) |
Responses |
Re: new initdb.c available
Re: new initdb.c available |
List | pgsql-hackers |
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. > > >
pgsql-hackers by date: