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

[Eric Sunshine <sunshine@sunshineco.com>]

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

> diff --git a/apply.c b/apply.c
> @@ -205,8 +205,13 @@ struct fragment {
> -#define BINARY_DELTA_DEFLATED  1
> -#define BINARY_LITERAL_DEFLATED 2
> +/* #define BINARY_DELTA_DEFLATED   1 */
> +/* #define BINARY_LITERAL_DEFLATED 2 */
> +
> +enum binary_type_deflated {
> +       BINARY_DELTA_DEFLATED = 1,
> +       BINARY_LITERAL_DEFLATED
> +};

As above, omit the commented-out #define lines. They serve no purpose.

> @@ -918,8 +923,14 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,
> -#define DIFF_OLD_NAME 0
> -#define DIFF_NEW_NAME 1
> +
> +/* #define DIFF_OLD_NAME 0 */
> +/* #define DIFF_NEW_NAME 1 */
> +
> +enum diff_name {
> +       DIFF_OLD_NAME = 0,
> +       DIFF_NEW_NAME
> +};

Unlike the change to "alias.c", these changes don't seem to be
harmful, thus they may make sense. However, as mentioned above, they
are not helpful to the debugger as-is. You would also have to change
the variable declarations from `int` to `binary_type_deflated` and
`diff_name`, respectively, for the debugger to benefit from the
change.