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

[Junio C Hamano <gitster@pobox.com>]

Æ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".

Thanks.