Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] pg_ctl

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: "Joshua D(dot) Drake" <jd(at)commandprompt(dot)com>,PostgreSQL Win32 port list <pgsql-hackers-win32(at)postgresql(dot)org>,PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [PATCHES] pg_ctl
Date: 2003-11-29 01:54:06
Message-ID: 200311290154.hAT1s6A14384@candle.pha.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackers-win32pgsql-patches
Yea, I have to agree.  It looks like it was written by an MS C
programmer, not by a PostgreSQL programmer in terms of style and
structure.

Sorry for the bad news.

---------------------------------------------------------------------------

Neil Conway wrote:
> This code is pretty awful, IMHO.
> 
> If you're going to copy code from a 3rd party (in this case, MSDN), it
> is standard practise to include an attribution. Also, what
> redistribution terms apply to MSDN sample code?
> 
> Assuming we don't remove or rewrite the MSDN sample code, it is
> usually considered good practise to put imported code into a separate
> file.
> 
> You can remove the kludge for 16-bit processes, as PostgreSQL won't
> ever be one. Dunno if it's worth modifying the sample code for this.
> 
> It is generally considered good practise to divide distinct
> functionality into separate functions, rather than use a ~450 line
> main() function.
> 
> I'd like to see fewer WIN32 #ifdefs in the main code path; please also
> endeavour to make them as small as possible.
> 
> Copying and pasting code from one branch of an #ifdef or if into the
> other should also be avoided where possible: this is done in a couple
> places.
> 
> Other random badness I noticed while browsing through the code:
> 
>     #ifdef WIN32
>     char *bindir="c:\\pgsql\\bin";
>     char *VERSION="7.3.5";
>     #else
>     char *bindir="@bindir@";
>     char *VERSION="@VERSION@";
>     #endif
> 
> This is plainly wrong.
> 
>     char* DEFPOSTOPTS= NULL;
>     char* POSTOPTSFILE= NULL;
>     char* PIDFILE= NULL;
> 
>     int n_sig;
> 
>     char *logfile=NULL;
>     char *silence_echo=NULL;
>     char *shutdown_mode="smart";
>     char *op=NULL;
>     int PID;
> 
> Can we please try to follow *some* kind of coherent convention for
> naming and indentation?
> 
>     char* buffer;
>     char* buffer2;
> 
> Please put some thought into your variable names.
> 
>     po_path=(char*)malloc(sizeof(char)* (strlen(PGPATH)+strlen(pgsqlexe)+2));
>     sprintf(po_path, "%s%s%s",PGPATH, path_delim, pgsqlexe);
> 
> This code needlessly assumes strlen(path_delim) == 1. On the other
> hand, it is reasonable to assume sizeof(char) == 1 (this is guaranteed
> by ANSI C).
> 
>     for ( ;cont; token = (char*)strtok(NULL, env_delim)){
>         if (token==NULL) cont=0; else {
>             buffer=(char*)malloc(sizeof(char)* (strlen(token)+strlen(CMDNAME)+1));
>             sprintf(buffer, token);
> 			strcat (buffer, path_delim);
> 			strcat (buffer, CMDNAME);
> 			if (post_file=fopen(buffer, "r")){
>                 self_path=token;
> 				cont=0;
> 				free(buffer);
> 			} else free(buffer);
> 		}
> 		if (token==NULL) cont=0;
> 	}
> 
> Good lord.
> 
> -Neil
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman(at)candle(dot)pha(dot)pa(dot)us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

In response to

pgsql-patches by date

Next:From: Joshua D. DrakeDate: 2003-11-29 04:02:09
Subject: Re: [PATCHES] pg_ctl
Previous:From: Neil ConwayDate: 2003-11-29 00:25:58
Subject: Re: [PATCHES] pg_ctl

pgsql-hackers-win32 by date

Next:From: Joshua D. DrakeDate: 2003-11-29 04:02:09
Subject: Re: [PATCHES] pg_ctl
Previous:From: Neil ConwayDate: 2003-11-29 00:25:58
Subject: Re: [PATCHES] pg_ctl

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group