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

On 14/03/2021 21:25, Martin Ågren wrote:

On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
[off-list ref] wrote:

quoted

From: Andrzej Hunt <redacted>

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.

quoted

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

Makes sense.

quoted

This changee was originally suggested by Peff in:

s/changee/change/

quoted

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

quoted

  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.

quoted

  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.

quoted

-       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.)

Thanks for catching this. Indeed I don't think any code relies on the specific values - but I don't feel comfortable making these changes silently or accidentally.

I will follow your suggested approach: I'll update this patch to retain the original values - and later on I'll grab the unused value for my new flag. That way *if* the new value were to cause a regression, it will be much more obvious where it came from.

ATB,
    Andrzej