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

> On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> > 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.
>
> This is something that I missed, but shouldn't have. I will change the
> type of the
> variable to enum and run the tests again, as you have mentioned.

> > Finally, please wrap the commit message so it fits in 72 columns.
>
> I am really sorry for this error, I was looking to be more descriptive
> in the commit
> message, but it does seem unnecessary to be this verbose.

> > 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".
>
> OK, I will do so. These technical specifications related to changes in alias.c
> are things that I did study about in C, but never actually saw them in
> practice, so
> I guess I just lowered my guard for such mistakes.