Re: [pgsql-hackers-win32] pg_ctl - Mailing list pgsql-patches

Hello,

   I am addressing these issues with my programmers now. Should see
better results next week.

Sincerely,

Joshua D. Drake


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
>
>


--
Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-222-2783 - jd@commandprompt.com - http://www.commandprompt.com
Editor-N-Chief - PostgreSQl.Org - http://www.postgresql.org