{"thread":{"id":"59219","subject":"[GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums","startedAt":"2023-02-09T16:46:25Z","lastAt":"2023-02-14T23:51:10Z","messageCount":4,"participants":["Vinayak Dev","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471869","messageId":"20230209164552.8971-1-vinayakdev.sci@gmail.com","threadId":"59219","inReplyTo":null,"subject":"[GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-09T16:45:52Z","receivedAt":"2023-02-09T16:46:25Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"Groups related #define macros into enums in apply.c and alias.c .\nThe changed #define macros are related constants, so it makes sense to group them.\nThis also makes debugging easier, as modern debuggers can identify enum constants and list them accordingly.\n\nSigned-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n\nvinayakdsci (1):\n  Change #define to enum in apply.c and alias.c\n\n alias.c | 12 +++++++++---\n apply.c | 19 +++++++++++++++----\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\n\ndiff --git a/alias.c b/alias.c\nindex 00abde0817..61ef2c0c54 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)\n \tread_early_config(config_alias_cb, &data);\n }\n \n-#define SPLIT_CMDLINE_BAD_ENDING 1\n-#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n-#define SPLIT_CMDLINE_ARGC_OVERFLOW 3\n+/* #define SPLIT_CMDLINE_BAD_ENDING 1 */\n+/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */\n+/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */\n+enum split_cmdline_error {\n+\tSPLIT_CMDLINE_BAD_ENDING = 1,\n+\tSPLIT_CMDLINE_UNCLOSED_QUOTE,\n+\tSPLIT_CMDLINE_ARGC_OVERFLOW\n+};\n+\n static const char *split_cmdline_errors[] = {\n \tN_(\"cmdline ends with \\\\\"),\n \tN_(\"unclosed quote\"),\ndiff --git a/apply.c b/apply.c\nindex 5eec433583..1e9cf2f4f2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -205,8 +205,13 @@ struct fragment {\n  * or deflated \"literal\".\n  */\n #define binary_patch_method leading\n-#define BINARY_DELTA_DEFLATED\t1\n-#define BINARY_LITERAL_DEFLATED 2\n+/* #define BINARY_DELTA_DEFLATED   1 */\n+/* #define BINARY_LITERAL_DEFLATED 2 */\n+\n+enum binary_type_deflated {\n+\tBINARY_DELTA_DEFLATED = 1,\n+\tBINARY_LITERAL_DEFLATED\n+};\n \n static void free_fragment_list(struct fragment *list)\n {\n@@ -918,8 +923,14 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,\n  * their names against any previous information, just\n  * to make sure..\n  */\n-#define DIFF_OLD_NAME 0\n-#define DIFF_NEW_NAME 1\n+\n+/* #define DIFF_OLD_NAME 0 */\n+/* #define DIFF_NEW_NAME 1 */\n+\n+enum diff_name {\n+\tDIFF_OLD_NAME = 0,\n+\tDIFF_NEW_NAME\n+};\n \n static int gitdiff_verify_name(struct gitdiff_data *state,\n \t\t\t       const char *line,\n-- \n2.39.1\n\n\n"},{"id":"471909","messageId":"CAPig+cT1EtPO2FLvTsw3SjgCgk=ovNwY77hsX6p7ETKiq8aNog@mail.gmail.com","threadId":"59219","inReplyTo":"20230209164552.8971-1-vinayakdev.sci@gmail.com","subject":"Re: [GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-10T10:45:35Z","receivedAt":"2023-02-10T10:45:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> {apply,alias}: convert pre-processor macros to enums\n\nThanks for submitting a GSoC microproject. Let's take a look...\n\n> Groups related #define macros into enums in apply.c and alias.c .\n\ns/Groups/Group/\n\n> The changed #define macros are related constants, so it makes sense to group them.\n\nThey already have a common prefix and are grouped textually, so it's\nobvious to readers that they are related, thus this isn't necessarily\na good selling point for such a change.\n\n> This also makes debugging easier, as modern debuggers can identify enum constants and list them accordingly.\n\nThis is a much better selling point for why such a change would be\ndesirable. Unfortunately, though, the real situation is more\ncomplicated. The stated argument is only true if these enum values are\nassigned to variables of the enum type. However, this patch only\ndefines the new enumeration type but never actually uses it to declare\nvariables, so the benefit for the debugger is never seen. For\ninstance, this patch defines:\n\n  enum binary_type_deflated {\n    BINARY_DELTA_DEFLATED = 1,\n    BINARY_LITERAL_DEFLATED\n  };\n\nbut then the code only ever assigns the enum value to an 'int' variable:\n\n  int patch_method;\n  ...\n  patch_method = BINARY_DELTA_DEFLATED;\n\nat which point the enum value's type is lost; it's an `int` and that's\nhow the debugger sees it, just as an `int`, so the debugger\ncan't/won't show it as an actual enum value.\n\nTo fix this, the patch would need to change the variable declaration, as well:\n\n  enum binary_type_deflated enum binary_type_deflated;\n\nFinally, please wrap the commit message so it fits in 72 columns.\n\n> Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n>\n> vinayakdsci (1):\n>   Change #define to enum in apply.c and alias.c\n\nThere should be a \"---\" line just below your sign-off to tell git-am\nwhere the patch's commit message ends, but the \"---\" line is missing\nfor some reason. If you didn't remove the \"---\" line manually, then\nyou may need to adjust your patch-sending tool to not strip it out.\n\n> diff --git a/alias.c b/alias.c\n> @@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)\n> -#define SPLIT_CMDLINE_BAD_ENDING 1\n> -#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n> -#define SPLIT_CMDLINE_ARGC_OVERFLOW 3\n> +/* #define SPLIT_CMDLINE_BAD_ENDING 1 */\n> +/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */\n> +/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */\n> +enum split_cmdline_error {\n> +       SPLIT_CMDLINE_BAD_ENDING = 1,\n> +       SPLIT_CMDLINE_UNCLOSED_QUOTE,\n> +       SPLIT_CMDLINE_ARGC_OVERFLOW\n> +};\n\nRetaining the #define lines as comments serves no purpose once you\nhave introduced the enum. It's confusing for readers, and there is a\ngood chance that the commented-out #defines and enum values will drift\nout of sync over time.\n\nIf we look at the code which utilizes these values, we see several\ninstances like this:\n\n  return -SPLIT_CMDLINE_BAD_ENDING;\n\nwhich means that the value being returned from the function is not a\nvalid enum value since it has been negated. Thus, in this case,\nconverting the #defines to an enum makes the code less valid and less\nclear. Moreover, these constants are only used in `return` statements\nfrom the function, are always negated, and are always returned as the\nexit code of the program itself; they are never stored in variables.\nThus, the debugger-related benefit mentioned in the commit message can\nnever materialize.\n\nSo, all in all, I'd say that this is one of those unfortunate cases in\nwhich conversion from #define to enum is unwanted since it makes the\ncode less clear and less valid, and provides no benefit. If you reroll\nthe patch, I'd suggest dropping these modifications to \"alias.c\".\n\n> diff --git a/apply.c b/apply.c\n> @@ -205,8 +205,13 @@ struct fragment {\n> -#define BINARY_DELTA_DEFLATED  1\n> -#define BINARY_LITERAL_DEFLATED 2\n> +/* #define BINARY_DELTA_DEFLATED   1 */\n> +/* #define BINARY_LITERAL_DEFLATED 2 */\n> +\n> +enum binary_type_deflated {\n> +       BINARY_DELTA_DEFLATED = 1,\n> +       BINARY_LITERAL_DEFLATED\n> +};\n\nAs above, omit the commented-out #define lines. They serve no purpose.\n\n> @@ -918,8 +923,14 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,\n> -#define DIFF_OLD_NAME 0\n> -#define DIFF_NEW_NAME 1\n> +\n> +/* #define DIFF_OLD_NAME 0 */\n> +/* #define DIFF_NEW_NAME 1 */\n> +\n> +enum diff_name {\n> +       DIFF_OLD_NAME = 0,\n> +       DIFF_NEW_NAME\n> +};\n\nUnlike the change to \"alias.c\", these changes don't seem to be\nharmful, thus they may make sense. However, as mentioned above, they\nare not helpful to the debugger as-is. You would also have to change\nthe variable declarations from `int` to `binary_type_deflated` and\n`diff_name`, respectively, for the debugger to benefit from the\nchange.\n"},{"id":"471913","messageId":"CADE8NappDSaZrMLeqak4is59oL=X1wJOj2eCLLjaCKyrnoK9PQ@mail.gmail.com","threadId":"59219","inReplyTo":"CAPig+cT1EtPO2FLvTsw3SjgCgk=ovNwY77hsX6p7ETKiq8aNog@mail.gmail.com","subject":"Re: [GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-10T11:20:21Z","receivedAt":"2023-02-10T11:20:38Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\nThanks a lot for replying!\n\n> On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> > {apply,alias}: convert pre-processor macros to enums\n>\n> Thanks for submitting a GSoC microproject. Let's take a look...\n>\n> > Groups related #define macros into enums in apply.c and alias.c .\n>\n> s/Groups/Group/\n\nThanks for pointing this out. I will keep note of removing the /s/ in\nfurther patches.\n\n>\n> > The changed #define macros are related constants, so it makes sense to group them.\n>\n> They already have a common prefix and are grouped textually, so it's\n> obvious to readers that they are related, thus this isn't necessarily\n> a good selling point for such a change.\n>\n> > This also makes debugging easier, as modern debuggers can identify enum constants and list them accordingly.\n>\n> This is a much better selling point for why such a change would be\n> desirable. Unfortunately, though, the real situation is more\n> complicated. The stated argument is only true if these enum values are\n> assigned to variables of the enum type. However, this patch only\n> defines the new enumeration type but never actually uses it to declare\n> variables, so the benefit for the debugger is never seen. For\n> instance, this patch defines:\n>\n>   enum binary_type_deflated {\n>     BINARY_DELTA_DEFLATED = 1,\n>     BINARY_LITERAL_DEFLATED\n>   };\n>\n> but then the code only ever assigns the enum value to an 'int' variable:\n>\n>   int patch_method;\n>   ...\n>   patch_method = BINARY_DELTA_DEFLATED;\n>\n> at which point the enum value's type is lost; it's an `int` and that's\n> how the debugger sees it, just as an `int`, so the debugger\n> can't/won't show it as an actual enum value.\n\nThis is something that I missed, but shouldn't have. I will change the\ntype of the\nvariable to enum and run the tests again, as you have mentioned.\n\n>\n> To fix this, the patch would need to change the variable declaration, as well:\n>\n>   enum binary_type_deflated enum binary_type_deflated;\n>\n> Finally, please wrap the commit message so it fits in 72 columns.\n\nI am really sorry for this error, I was looking to be more descriptive\nin the commit\nmessage, but it does seem unnecessary to be this verbose.\n\n>\n> > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n> >\n> > vinayakdsci (1):\n> >   Change #define to enum in apply.c and alias.c\n>\n> There should be a \"---\" line just below your sign-off to tell git-am\n> where the patch's commit message ends, but the \"---\" line is missing\n> for some reason. If you didn't remove the \"---\" line manually, then\n> you may need to adjust your patch-sending tool to not strip it out.\n>\n\nI will correct this error in further patches, it looks like I did\nremove it erroneously.\n\n> > diff --git a/alias.c b/alias.c\n> > @@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)\n> > -#define SPLIT_CMDLINE_BAD_ENDING 1\n> > -#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n> > -#define SPLIT_CMDLINE_ARGC_OVERFLOW 3\n> > +/* #define SPLIT_CMDLINE_BAD_ENDING 1 */\n> > +/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */\n> > +/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */\n> > +enum split_cmdline_error {\n> > +       SPLIT_CMDLINE_BAD_ENDING = 1,\n> > +       SPLIT_CMDLINE_UNCLOSED_QUOTE,\n> > +       SPLIT_CMDLINE_ARGC_OVERFLOW\n> > +};\n>\n> Retaining the #define lines as comments serves no purpose once you\n> have introduced the enum. It's confusing for readers, and there is a\n> good chance that the commented-out #defines and enum values will drift\n> out of sync over time.\n>\n\nI did not remove them in fear of making a mistake in the enum\nfields(rookie mistake), but I should\nhave removed them, your point is absolutely valid.\n\n> If we look at the code which utilizes these values, we see several\n> instances like this:\n>\n>   return -SPLIT_CMDLINE_BAD_ENDING;\n>\n> which means that the value being returned from the function is not a\n> valid enum value since it has been negated. Thus, in this case,\n> converting the #defines to an enum makes the code less valid and less\n> clear. Moreover, these constants are only used in `return` statements\n> from the function, are always negated, and are always returned as the\n> exit code of the program itself; they are never stored in variables.\n> Thus, the debugger-related benefit mentioned in the commit message can\n> never materialize.\n>\n> So, all in all, I'd say that this is one of those unfortunate cases in\n> which conversion from #define to enum is unwanted since it makes the\n> code less clear and less valid, and provides no benefit. If you reroll\n> the patch, I'd suggest dropping these modifications to \"alias.c\".\n>\n\nOK, I will do so. These technical specifications related to changes in alias.c\nare things that I did study about in C, but never actually saw them in\npractice, so\nI guess I just lowered my guard for such mistakes.\n\nFurther, I will change variable types from int to enum in apply.c, which,\nas you correctly point out, are still integers.\nI will amend these mistakes in further patches.\n\nThanks a lot!\nVinayak\n"},{"id":"472099","messageId":"CAPig+cT4TS0jn8LgjuV+N-aK+U0X6NW02gjMi=mi_9D204KHmg@mail.gmail.com","threadId":"59219","inReplyTo":"CADE8NappDSaZrMLeqak4is59oL=X1wJOj2eCLLjaCKyrnoK9PQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] {apply,alias}: convert pre-processor macros to enums","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-14T23:50:39Z","receivedAt":"2023-02-14T23:51:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 10, 2023 at 6:20 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> > This is a much better selling point for why such a change would be\n> > desirable. Unfortunately, though, the real situation is more\n> > complicated. The stated argument is only true if these enum values are\n> > assigned to variables of the enum type. However, this patch only\n> > defines the new enumeration type but never actually uses it to declare\n> > variables, so the benefit for the debugger is never seen. For\n> > instance, this patch defines:\n> >\n> >   enum binary_type_deflated {\n> >     BINARY_DELTA_DEFLATED = 1,\n> >     BINARY_LITERAL_DEFLATED\n> >   };\n> >\n> > but then the code only ever assigns the enum value to an 'int' variable:\n> >\n> >   int patch_method;\n> >   ...\n> >   patch_method = BINARY_DELTA_DEFLATED;\n> >\n> > at which point the enum value's type is lost; it's an `int` and that's\n> > how the debugger sees it, just as an `int`, so the debugger\n> > can't/won't show it as an actual enum value.\n>\n> This is something that I missed, but shouldn't have. I will change the\n> type of the\n> variable to enum and run the tests again, as you have mentioned.\n\nThis is what the review process is for. Review comments are not saying\n\"you should have caught this yourself\"; the intention of review\ncomments is to help you improve the patch. Even well-seasoned\ncontributors make mistakes, but fortunately many of those mistakes get\ncaught while the patch is still in the review phase.\n\n> > Finally, please wrap the commit message so it fits in 72 columns.\n>\n> I am really sorry for this error, I was looking to be more descriptive\n> in the commit\n> message, but it does seem unnecessary to be this verbose.\n\nNo need to apologize. As mentioned above, review comments are meant to\nhelp you improve the patch; they are not meant to place blame.\n\nIt's good to write descriptive commit messages. My comment about \"72\ncolumns\" was merely asking you to word-wrap the commit message so it\nfits nicely on a page.\n\n> > If we look at the code which utilizes these values, we see several\n> > instances like this:\n> >\n> >   return -SPLIT_CMDLINE_BAD_ENDING;\n> >\n> > which means that the value being returned from the function is not a\n> > valid enum value since it has been negated. Thus, in this case,\n> > converting the #defines to an enum makes the code less valid and less\n> > clear. Moreover, these constants are only used in `return` statements\n> > from the function, are always negated, and are always returned as the\n> > exit code of the program itself; they are never stored in variables.\n> > Thus, the debugger-related benefit mentioned in the commit message can\n> > never materialize.\n> >\n> > So, all in all, I'd say that this is one of those unfortunate cases in\n> > which conversion from #define to enum is unwanted since it makes the\n> > code less clear and less valid, and provides no benefit. If you reroll\n> > the patch, I'd suggest dropping these modifications to \"alias.c\".\n>\n> OK, I will do so. These technical specifications related to changes in alias.c\n> are things that I did study about in C, but never actually saw them in\n> practice, so\n> I guess I just lowered my guard for such mistakes.\n\nThe review process is about sharing knowledge. Hopefully my\nexplanation of why this change is unwanted made sense.\n"}]}