diff mbox series

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

Message ID 2b03785bd4cb76285989aff259af57890ea9fe08.1615747662.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix all leaks in t0001 | expand

Commit Message

Andrzej Hunt March 14, 2021, 6:47 p.m. UTC 
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.

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

This changee was originally suggested by Peff in:
https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 parse-options.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Martin Ågren March 14, 2021, 8:25 p.m. UTC | #1 
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.)

Martin
Junio C Hamano March 14, 2021, 10:57 p.m. UTC | #2 
Martin Ågren <martin.agren@gmail.com> writes:

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

I think 8 vs 1<<3 is bikeshedding, but a patch that claims to
"convert bitfield flags to use binary shift" is supposed to be a
no-op, so it would highly be problematic to see such a hidden
renumbering happening at the same time.  Even if the existing code
or any code in flight only relies on the fact that these bits are
distinct and non overlapping.

Thanks for a sharp set of eyes.
Andrzej Hunt March 15, 2021, 4:20 p.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/parse-options.h b/parse-options.h
index ff6506a50470..36ce0a44b2e9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -28,26 +28,26 @@  enum parse_opt_type {
 };
 
 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,
 };
 
 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_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,
 };
 
 enum parse_opt_result {