apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] apr_getopt_long interface update and interleaving support
Date Sat, 25 Nov 2000 00:20:47 GMT
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