Date: Tue, 5 Jun 2012 18:40:37 +1000
Message-ID: <20120605184037.GB14297@hili.localdomain>
From: Peter Wang <novalazy@gmail.com>
To: notmuch@notmuchmail.org
Subject: Re: [PATCH] cli: make the command line parser's errors more
	informative.
In-Reply-To: <1338724128-13158-1-git-send-email-markwalters1009@gmail.com>
References: <1338723972-13063-1-git-send-email-markwalters1009@gmail.com>
	<1338724128-13158-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Precedence: list

On Sun,  3 Jun 2012 12:48:48 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>  
> +static notmuch_bool_t
> +_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
> +
> +    char *endptr;
> +    if (next == 0 || arg_str[0] == 0) {
> +	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
> +	return FALSE;
> +    }
> +
> +    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
> +    if (*endptr == 0)
> +	return TRUE;

It's usually clearer to write '\0' for the null character.

> @@ -99,20 +133,13 @@ parse_option (const char *arg,
>  	    char next = arg[strlen (try->name)];
>  	    const char *value= arg+strlen(try->name)+1;
>  
> -	    char *endptr;
> -
> -	    /* Everything but boolean arguments (switches) needs a
> -	     * delimiter, and a non-zero length value. Boolean
> -	     * arguments may take an optional =true or =false value.
> -	     */
> -	    if (next != '=' && next != ':' && next != 0) return FALSE;
> -	    if (next == 0) {
> -		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
> -		    try->opt_type != NOTMUCH_OPT_KEYWORD)
> -		    return FALSE;
> -	    } else {
> -		if (value[0] == 0) return FALSE;
> -	    }
> +	    /* If this is not the end of the argument (i.e. the next
> +	     * character is not a space or a delimiter) we stop
> +	     * parsing for this option but allow the parsing to
> +	     * continue to for other options. This should allow
> +	     * options to be initial segments of other options. */

It took me a little while to figure out what the last sentence was
about.  Perhaps:

    If we have not reached the end of the argument (i.e. the next
    character is not a space or delimiter) then the argument could
    still match a longer option name later in the option table.

(otherwise, "continue to for other")

> +	    if (next != '=' && next != ':' && next != 0)
> +		goto DONE_THIS_OPTION;

The `goto' could be expressed as a `continue' in a `for' loop, AFAICS.

>  
>  	    if (try->output_var == NULL)
>  		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
> @@ -125,12 +152,10 @@ parse_option (const char *arg,
>  		return _process_boolean_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_INT:
> -		*((int *)try->output_var) = strtol (value, &endptr, 10);
> -		return (*endptr == 0);
> +		return _process_int_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_STRING:
> -		*((const char **)try->output_var) = value;
> -		return TRUE;
> +		return _process_string_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_POSITION:
>  	    case NOTMUCH_OPT_END:
> @@ -139,6 +164,7 @@ parse_option (const char *arg,
>  		/*UNREACHED*/
>  	    }
>  	}
> +    DONE_THIS_OPTION:
>  	try++;
>      }
>      fprintf (stderr, "Unrecognized option: --%s\n", arg);

Peter