Re: [PATCH 02/10] parse-options.[ch]: consistently use "enum parse_opt_flags"

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Use the "enum parse_opt_flags" instead of an "int flags" as arguments
>> to the various functions in parse-options.c. This will help to catch
>> cases where we're not handling cases in switch statements, and
>
> I am not sure about most part this change, and the claim the second
> sentence makes is certainly dubious.  Let's look at the first hunk:
>
>> diff --git a/parse-options.c b/parse-options.c
>> index 55c5821b08d..9dce8b7f8a8 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)
>
>  static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
>  				  int argc, const char **argv, const char *prefix,
> -				  const struct option *options, int flags)
> +				  const struct option *options,
> +				  enum parse_opt_flags flags)
>  {
>  	ctx->argc = argc;
>  	ctx->argv = argv;
>  	if (!(flags & PARSE_OPT_ONE_SHOT)) {
>  		ctx->argc--;
>  		ctx->argv++;
>  	}
>  	ctx->total = ctx->argc;
>  	ctx->out   = argv;
>  	ctx->prefix = prefix;
>  	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
>  	ctx->flags = flags;
>  	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
>  	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
>  	    !(flags & PARSE_OPT_ONE_SHOT))
>  		BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
>  	if ((flags & PARSE_OPT_ONE_SHOT) &&
>  	    (flags & PARSE_OPT_KEEP_ARGV0))
>  		BUG("Can't keep argv0 if you don't have it");
>  	parse_options_check(options);
>  }
>  
> The "flags" parameter does not take a value that is an "enum" in the
> usual "enumeration" sense at all.  Even though parse_opt_flags
> defines 7 distinct "enum" values, each enumerated value is a small
> unsigned integer with only single bit set, the caller can throw a
> value that is not among these 7 by OR'ing them together.  We would
> not sensibly do
>
> 	switch (flags) {
> 	case PARSE_OPT_KEEP_UNKNOWN:
> 		...
>
> In general, I am not all that enthusiastic for such a(n) (ab)use of
> "enum" for bit patterns, much less than "enumerate all possible
> values to make sure compilers would help us catch missing logic".

> The "parse_opt_result" enum is the "right" kind of enumeration that
> I can stand behind fully.  The hunk related to that enum in this
> patch is quite reasonable and takes advantage of the fact that the
> enum is meant to be the enumeration of all possible values.
>
> Compared to it, parse_opt_flags and parse_opt_option_flags, not
> really.
>
> If we wanted to really clean up the latter two, perhaps we should
> define the bit (which *can* be made to a proper "here are the all
> possible values" enumeration), like this:
>
>     enum parse_opt_flags_bit {
> 	PARSE_OPT_KEEP_DASHDASH_BIT = 0,
>         PARSE_OPT_STOP_AT_NON_OPTION_BIT,
> 	PARSE_OPT_KEEP_ARGV0_BIT,
> 	...
> 	PARSE_OPT_SHELL_EVAL_BIT,
>     };
>
> and then update the users to use (1U << PARSE_OPT_$FLAG$_BIT), or
> drop the pretense that it is a good idea to use enum for bit pattern
> and use the CPP macro, i.e.
>
>     #define PARSE_OPT_KEEP_DASHDASH (1U<<0)
>     #define PARSE_OPT_STOP_AT_NON_OPTION (1U<<1)
>     ...
>     #define PARSE_OPT_SHELL_EVAL (1U<<6)
>
> to make it clear that we do not mean these are "enumeration of
> possible values".