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/
Received on Sat Oct 21 14:36:15 2006