From: Andrzej Hunt <andrzej@ahunt.org>
Date: Mon, 15 Mar 2021 16:20:20 GMT
Subject: Re: [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift
Message-ID: <66839f46-9c17-db79-ba9a-54472ed9c249@ahunt.org>
In-Reply-To: <CAN0heSo_ArBrjSncvx==kw8K1DzH=0y2h-Q=B_364Ea0EURXxg@mail.gmail.com>

On 14/03/2021 21:25, Martin Ågren wrote:
> On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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/
> 
>>   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.
> 
>>   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.
> 
>> -       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