Re: [GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums

> On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> > {apply,alias}: convert pre-processor macros to enums
>
> Thanks for submitting a GSoC microproject. Let's take a look...
>
> > Groups related #define macros into enums in apply.c and alias.c .
>
> s/Groups/Group/

>
> > The changed #define macros are related constants, so it makes sense to group them.
>
> They already have a common prefix and are grouped textually, so it's
> obvious to readers that they are related, thus this isn't necessarily
> a good selling point for such a change.
>
> > This also makes debugging easier, as modern debuggers can identify enum constants and list them accordingly.
>
> This is a much better selling point for why such a change would be
> desirable. Unfortunately, though, the real situation is more
> complicated. The stated argument is only true if these enum values are
> assigned to variables of the enum type. However, this patch only
> defines the new enumeration type but never actually uses it to declare
> variables, so the benefit for the debugger is never seen. For
> instance, this patch defines:
>
>   enum binary_type_deflated {
>     BINARY_DELTA_DEFLATED = 1,
>     BINARY_LITERAL_DEFLATED
>   };
>
> but then the code only ever assigns the enum value to an 'int' variable:
>
>   int patch_method;
>   ...
>   patch_method = BINARY_DELTA_DEFLATED;
>
> at which point the enum value's type is lost; it's an `int` and that's
> how the debugger sees it, just as an `int`, so the debugger
> can't/won't show it as an actual enum value.

>
> To fix this, the patch would need to change the variable declaration, as well:
>
>   enum binary_type_deflated enum binary_type_deflated;
>
> Finally, please wrap the commit message so it fits in 72 columns.

>
> > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
> >
> > vinayakdsci (1):
> >   Change #define to enum in apply.c and alias.c
>
> There should be a "---" line just below your sign-off to tell git-am
> where the patch's commit message ends, but the "---" line is missing
> for some reason. If you didn't remove the "---" line manually, then
> you may need to adjust your patch-sending tool to not strip it out.
>

> > diff --git a/alias.c b/alias.c
> > @@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)
> > -#define SPLIT_CMDLINE_BAD_ENDING 1
> > -#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
> > -#define SPLIT_CMDLINE_ARGC_OVERFLOW 3
> > +/* #define SPLIT_CMDLINE_BAD_ENDING 1 */
> > +/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */
> > +/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */
> > +enum split_cmdline_error {
> > +       SPLIT_CMDLINE_BAD_ENDING = 1,
> > +       SPLIT_CMDLINE_UNCLOSED_QUOTE,
> > +       SPLIT_CMDLINE_ARGC_OVERFLOW
> > +};
>
> Retaining the #define lines as comments serves no purpose once you
> have introduced the enum. It's confusing for readers, and there is a
> good chance that the commented-out #defines and enum values will drift
> out of sync over time.
>

> If we look at the code which utilizes these values, we see several
> instances like this:
>
>   return -SPLIT_CMDLINE_BAD_ENDING;
>
> which means that the value being returned from the function is not a
> valid enum value since it has been negated. Thus, in this case,
> converting the #defines to an enum makes the code less valid and less
> clear. Moreover, these constants are only used in `return` statements
> from the function, are always negated, and are always returned as the
> exit code of the program itself; they are never stored in variables.
> Thus, the debugger-related benefit mentioned in the commit message can
> never materialize.
>
> So, all in all, I'd say that this is one of those unfortunate cases in
> which conversion from #define to enum is unwanted since it makes the
> code less clear and less valid, and provides no benefit. If you reroll
> the patch, I'd suggest dropping these modifications to "alias.c".
>