Re: new initdb.c available

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 peter_e(at)gmx(dot)net