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

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

[Vinayak Dev]

Groups related #define macros into enums in apply.c and alias.c .
The changed #define macros are related constants, so it makes sense to group them.
This also makes debugging easier, as modern debuggers can identify enum constants and list them accordingly.

Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>

vinayakdsci (1):
  Change #define to enum in apply.c and alias.c

 alias.c | 12 +++++++++---
 apply.c | 19 +++++++++++++++----
 2 files changed, 24 insertions(+), 7 deletions(-)


diff --git a/alias.c b/alias.c
index 00abde0817..61ef2c0c54 100644
--- a/alias.c
+++ b/alias.c
@@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
-#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
+};
+
 static const char *split_cmdline_errors[] = {
 	N_("cmdline ends with \\"),
 	N_("unclosed quote"),
diff --git a/apply.c b/apply.c
index 5eec433583..1e9cf2f4f2 100644
--- a/apply.c
+++ b/apply.c
@@ -205,8 +205,13 @@ struct fragment {
  * or deflated "literal".
  */
 #define binary_patch_method leading
-#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
+};
 
 static void free_fragment_list(struct fragment *list)
 {
@@ -918,8 +923,14 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,
  * their names against any previous information, just
  * to make sure..
  */
-#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
+};
 
 static int gitdiff_verify_name(struct gitdiff_data *state,
 			       const char *line,
-- 
2.39.1

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

[Eric Sunshine]

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.

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

[Vinayak Dev]

On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@sunshineco.com> wrote:

Thanks a lot for replying!

> 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/

Thanks for pointing this out. I will keep note of removing the /s/ in
further patches.

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

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.

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

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.

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

I will correct this error in further patches, it looks like I did
remove it erroneously.

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

I did not remove them in fear of making a mistake in the enum
fields(rookie mistake), but I should
have removed them, your point is absolutely valid.

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

Further, I will change variable types from int to enum in apply.c, which,
as you correctly point out, are still integers.
I will amend these mistakes in further patches.

Thanks a lot!
Vinayak

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

[Eric Sunshine]

On Fri, Feb 10, 2023 at 6:20 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> 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.

This is what the review process is for. Review comments are not saying
"you should have caught this yourself"; the intention of review
comments is to help you improve the patch. Even well-seasoned
contributors make mistakes, but fortunately many of those mistakes get
caught while the patch is still in the review phase.

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

No need to apologize. As mentioned above, review comments are meant to
help you improve the patch; they are not meant to place blame.

It's good to write descriptive commit messages. My comment about "72
columns" was merely asking you to word-wrap the commit message so it
fits nicely on a page.

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

The review process is about sharing knowledge. Hopefully my
explanation of why this change is unwanted made sense.