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.
---------------------------(end of broadcast)--------------------------- TIP 4: Don't 'kill -9' the postmaster
