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@gmx.net