From gstein@lyra.org Sat Nov 25 00:19:51 2000 Return-Path: Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Delivered-To: mailing list dev@apr.apache.org Received: (qmail 85870 invoked from network); 25 Nov 2000 00:19:51 -0000 Received: from test.webdav.org (HELO kurgan.lyra.org) (198.144.203.199) by locus.apache.org with SMTP; 25 Nov 2000 00:19:51 -0000 Received: (from gstein@localhost) by kurgan.lyra.org (8.9.3/8.9.3) id QAA25746; Fri, 24 Nov 2000 16:20:47 -0800 X-Authentication-Warning: kurgan.lyra.org: gstein set sender to gstein@lyra.org using -f Date: Fri, 24 Nov 2000 16:20:47 -0800 From: Greg Stein To: Greg Hudson Cc: dev@apr.apache.org, dev@subversion.tigris.org Subject: Re: [PATCH] apr_getopt_long interface update and interleaving support Message-ID: <20001124162047.L21426@lyra.org> Mail-Followup-To: Greg Hudson , dev@apr.apache.org, dev@subversion.tigris.org References: <200011242346.SAA11788@egyptian-gods.MIT.EDU> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2i In-Reply-To: <200011242346.SAA11788@egyptian-gods.MIT.EDU>; from ghudson@mit.edu on Fri, Nov 24, 2000 at 06:46:25PM -0500 X-URL: http://www.lyra.org/greg/ X-Spam-Rating: locus.apache.org 1.6.2 0/1000/N Looks good overall! I've got a few nits, below. On Fri, Nov 24, 2000 at 06:46:25PM -0500, Greg Hudson wrote: >... > - char *const *argv; > + char **argv; Um. I don't think we can do this with argv. I'm surprised that it isn't "const char * const * argv". Certainly, if we make a copy of the array (of pointers), then we could have "const char **argv" and that would allow us to permute. >... > -typedef struct apr_getopt_long_t apr_getopt_long_t; > +typedef struct apr_option_t apr_option_t; > > -/* structure representing a single longopt */ > -struct apr_getopt_long_t { > - /** the name of the long argument (sans "--") */ > +struct apr_option_t { I think the name "apr_option_t" is a bit too generic. The apr_getopt_ prefix probably ought to remain. >... > @@ -106,7 +108,7 @@ > * @deffunc apr_status_t apr_initopt(apr_getopt_t **os, apr_pool_t *cont,int argc, char *const *argv) > */ > APR_DECLARE(apr_status_t) apr_initopt(apr_getopt_t **os, apr_pool_t *cont, > - int argc, char *const *argv); > + int argc, char **argv); This should be "const char * const * argv". We can make copies internally, if we choose. The changing prototype will have an effect upon Apache, but that isn't a problem... I can easily update those references. [ there may be some test proggies in apr/test/ that would need changing, too ] >... > @@ -57,6 +58,8 @@ > (*os)->place = EMSG; > (*os)->argc = argc; > (*os)->argv = argv; > + (*os)->interleave = 0; > + (*os)->skip_start = (*os)->skip_end = (*os)->ind; Can we simplify this and make it: (*os)->skip_start = 1; (*os)->skip_end = 1; By using the other variable, then I need to look back to figure out just what is going on. (and in this case, I didn't even have the context, so I needed to bring up the file to see what the above line meant) >... > +static void reverse(char **argv, int start, int len) > +{ > + char *temp; Constify on the above two lines... >... > + else if (os->ind >= os->argc) /* Argument missing */ > + return cerr(os, "option requires an argument", *optch, APR_BADARG); The error message is different from the similar error for long options. Cheers, -g -- Greg Stein, http://www.lyra.org/