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.
--
Peter Eisentraut [EMAIL PROTECTED]
---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
