Re: [PATCH] apr_getopt_long interface update and interleaving support

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/