Re: [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift

From: Martin Ågren <martin.agren@gmail.com>
Date: Mar 14, 2021, 20:25 UTC
Message-ID: <CAN0heSo_ArBrjSncvx==kw8K1DzH=0y2h-Q=B_364Ea0EURXxg@mail.gmail.com>
In-Reply-To: <2b03785bd4cb76285989aff259af57890ea9fe08.1615747662.git.gitgitgadget@gmail.com>

On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> Because it's easier to read, but also likely to be easier to maintain.
> I am making this change because I need to add a new flag in a later
> commit.

Makes sense.

> Also add a trailing comma to the last enum entry to simplify addition of
> new flags.

Makes sense.

> This changee was originally suggested by Peff in:

s/changee/change/

> https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/

Show 14 quoted lines

>  enum parse_opt_flags {
> -       PARSE_OPT_KEEP_DASHDASH = 1,
> -       PARSE_OPT_STOP_AT_NON_OPTION = 2,
> -       PARSE_OPT_KEEP_ARGV0 = 4,
> -       PARSE_OPT_KEEP_UNKNOWN = 8,
> -       PARSE_OPT_NO_INTERNAL_HELP = 16,
> -       PARSE_OPT_ONE_SHOT = 32
> +       PARSE_OPT_KEEP_DASHDASH = 1 << 0,
> +       PARSE_OPT_STOP_AT_NON_OPTION = 1 << 1,
> +       PARSE_OPT_KEEP_ARGV0 = 1 << 2,
> +       PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
> +       PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
> +       PARSE_OPT_ONE_SHOT = 1 << 5,
>  };

Straightforward.

Show 8 quoted lines

>  enum parse_opt_option_flags {
> -       PARSE_OPT_OPTARG  = 1,
> -       PARSE_OPT_NOARG   = 2,
> -       PARSE_OPT_NONEG   = 4,
> -       PARSE_OPT_HIDDEN  = 8,
> -       PARSE_OPT_LASTARG_DEFAULT = 16,
> -       PARSE_OPT_NODASH = 32,
> -       PARSE_OPT_LITERAL_ARGHELP = 64,

`PARSE_OPT_NEGHELP` is gone since acbb08c2e0b ("parse-options: remove PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.

Show 16 quoted lines

> -       PARSE_OPT_SHELL_EVAL = 256,
> -       PARSE_OPT_NOCOMPLETE = 512,
> -       PARSE_OPT_COMP_ARG = 1024,
> -       PARSE_OPT_CMDMODE = 2048
> +       PARSE_OPT_OPTARG  = 1 << 0,
> +       PARSE_OPT_NOARG   = 1 << 1,
> +       PARSE_OPT_NONEG   = 1 << 2,
> +       PARSE_OPT_HIDDEN  = 1 << 3,
> +       PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
> +       PARSE_OPT_NODASH = 1 << 5,
> +       PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
> +       PARSE_OPT_SHELL_EVAL = 1 << 7,
> +       PARSE_OPT_NOCOMPLETE = 1 << 8,
> +       PARSE_OPT_COMP_ARG = 1 << 9,
> +       PARSE_OPT_CMDMODE = 1 << 10,
>  };

Those last few conversions close the gap and we end with 1024 rather than 2048. That "should" be ok, unless some piece of code relies on exact values here, or even their relations(!). Hopefully not? Might be worth calling out in the commit message that you're changing some values, if you're rerolling anyway. (Or you could leave 1<<8 unused to make this a true no-op, then use that value in the next patch. Anyway, I think this is safely in bikeshedding land.)

Martin