apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Karl Fogel <kfo...@galois.collab.net>
Subject Re: [PATCH] apr_getopt_long interface update and interleaving support
Date Fri, 24 Nov 2000 22:08:02 GMT
Greg S, just coordinating:

Sounds like you're online and reviewing the changes already -- shall I
stay out of your way and assume you will do the commit soon?  (It's
not that I'm worried about a conflict or something.  It's just that I
will spend my time on other things than this change right now, if
you're on it anyway).

-K

Greg Stein <gstein@lyra.org> writes:
> 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/

Mime
View raw message