{"thread":{"id":"59245","subject":"[GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","startedAt":"2023-02-15T09:20:10Z","lastAt":"2023-02-16T17:21:23Z","messageCount":5,"participants":["Vinayak Dev","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"472128","messageId":"20230215091950.2976-1-vinayakdev.sci@gmail.com","threadId":"59245","inReplyTo":null,"subject":"[GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-15T09:19:50Z","receivedAt":"2023-02-15T09:20:10Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"From: vinayakdsci \n\nChange #define constants to enum and variable types from int to enum\nin apply.c\n\nEnum constants have an advantage over #define macro constants in that\nmodern debuggers are able to report them symbolically instead of just\nas simple numbers. This makes debugging and catching undercover errors\neasier, and can many a times save quite some time and inconvenience.\n\nSigned-off-by: Vinayak Dev \n\n---\n apply.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 5cc5479c9c..b2a03d9fc3 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -205,8 +205,10 @@ 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+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,14 +920,17 @@ 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+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 \t\t\t int isnull,\n \t\t\t char **name,\n-\t\t\t int side)\n+\t\t\t enum diff_name side)\n {\n \tif (!*name && !isnull) {\n \t\t*name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);\n@@ -1910,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n \tint llen, used;\n \tunsigned long size = *sz_p;\n \tchar *buffer = *buf_p;\n-\tint patch_method;\n+\tenum binary_type_deflated patch_method;\n \tunsigned long origlen;\n \tchar *data = NULL;\n \tint hunk_size = 0;\n\nbase-commit: b1485644f936ee83a995ec24d23f713f4230a1ae\n-- \n2.39.1\n\n"},{"id":"472149","messageId":"xmqqmt5ezun0.fsf@gitster.g","threadId":"59245","inReplyTo":"20230215091950.2976-1-vinayakdev.sci@gmail.com","subject":"Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-15T17:49:23Z","receivedAt":"2023-02-15T17:49:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vinayak Dev writes:\n\n> diff --git a/apply.c b/apply.c\n> index 5cc5479c9c..b2a03d9fc3 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -205,8 +205,10 @@ struct fragment {\n> * or deflated \"literal\".\n> */\n> #define binary_patch_method leading\n\nNotice and explain what this line is doing, perhaps?\n\n> -#define BINARY_DELTA_DEFLATED\t1\n> -#define BINARY_LITERAL_DEFLATED 2\n> +enum binary_type_deflated {\n> +\tBINARY_DELTA_DEFLATED = 1,\n> +\tBINARY_LITERAL_DEFLATED\n> +};\n\nThese days, we not just allow but encourage enum definitions to have\na trailing comma after the last item, UNLESS we want to signal that\nthe one at the end right now MUST stay to be the last one (e.g. a\nsentinel at the end).\n\nA patch that adds a new item to, removes an existing item from, or\nshuffles existing items in the list can be free of unnecessary patch\nnoise to omit the last comma.\n\nAs a faithful rewrite, forcing the same values to be given as before\nby saying that \"_DEFLATED must be 1\" is a good thing to do, but once\nthe dust settled from the patch, it would be a good idea to go back\nto the code and see if the values MUST be these, or if it is fine to\nuse any value as long as they are distinct. If it is the latter,\nthen it would make a good follow-up patch to remove \"= 1\", with an\nexplanation why it is a safe thing to do.\n\n> static void free_fragment_list(struct fragment *list)\n> {\n> @@ -918,14 +920,17 @@ 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> +enum diff_name {\n> +\tDIFF_OLD_NAME = 0,\n> +\tDIFF_NEW_NAME\n> +};\n\nDitto.\n\n> static int gitdiff_verify_name(struct gitdiff_data *state,\n> \t\t\t const char *line,\n> \t\t\t int isnull,\n> \t\t\t char **name,\n> -\t\t\t int side)\n> +\t\t\t enum diff_name side)\n> {\n> \tif (!*name && !isnull) {\n> \t\t*name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);\n> @@ -1910,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n> \tint llen, used;\n> \tunsigned long size = *sz_p;\n> \tchar *buffer = *buf_p;\n> -\tint patch_method;\n> +\tenum binary_type_deflated patch_method;\n\nThis is not quite sufficient to achieve the goal of helping \"modern\ndebuggers\" that was stated in the proposed log message, is it?\nparse_binary_hunk() copies the value from this local variable to a\nmember in the fragment being parsed, like so:\n\n\tfrag->binary_patch_method = patch_method;\n\nbut the thing is, as we have seen earlier, a compiler macro is used\nto (ab)use the \"leading\" member and call it \"binary_patch_method\".\nThe type of that member is \"unsigned long\".\n\nNow if our ultimate goal were to use enum instead of macro, then an\nobvious \"solution\" would be to stop abusing \"leading\". Instead, you\nwould add \"enum binary_type_deflated binary_patch_method\" member to\nthe fragment struct and use the enum throughout.\n\nBut is it worth it?\n\nUsing enum instead of macro is *NOT* a goal. If doing so makes our\ncode better, we may do so---which tells us that use of enum is not a\ngoal but a means to make our code better. Is adding an extra member\nthat is used only for binary patches improve our code? I dunno.\n\nThanks.\n"},{"id":"472200","messageId":"CADE8Nao=ZrF7_0+G=jB8=j5Aji4ndCLfMp-xjbSpc1HKmmFM7w@mail.gmail.com","threadId":"59245","inReplyTo":"xmqqmt5ezun0.fsf@gitster.g","subject":"Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-16T14:04:44Z","receivedAt":"2023-02-16T14:05:02Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"On Wed, 15 Feb 2023 at 23:19, Junio C Hamano wrote:\n>\n> Vinayak Dev writes:\n>\n> > diff --git a/apply.c b/apply.c\n> > index 5cc5479c9c..b2a03d9fc3 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -205,8 +205,10 @@ struct fragment {\n> > * or deflated \"literal\".\n> > */\n> > #define binary_patch_method leading\n>\n> Notice and explain what this line is doing, perhaps?\n\nIt does appear that the macro binary_patch_method actually refers to\nan unsigned long.\nI think leading represents the number of leading context lines in a\nhunk/fragment,\nand the variable is reused to store the type for a binary fragment.\n\n> > -#define BINARY_DELTA_DEFLATED 1\n> > -#define BINARY_LITERAL_DEFLATED 2\n> > +enum binary_type_deflated {\n> > + BINARY_DELTA_DEFLATED = 1,\n> > + BINARY_LITERAL_DEFLATED\n> > +};\n>\n> These days, we not just allow but encourage enum definitions to have\n> a trailing comma after the last item, UNLESS we want to signal that\n> the one at the end right now MUST stay to be the last one (e.g. a\n> sentinel at the end).\n\n> A patch that adds a new item to, removes an existing item from, or\n> shuffles existing items in the list can be free of unnecessary patch\n> noise to omit the last comma.\n\nOk. Point noted.\n\n> As a faithful rewrite, forcing the same values to be given as before\n> by saying that \"_DEFLATED must be 1\" is a good thing to do, but once\n> the dust settled from the patch, it would be a good idea to go back\n> to the code and see if the values MUST be these, or if it is fine to\n> use any value as long as they are distinct. If it is the latter,\n> then it would make a good follow-up patch to remove \"= 1\", with an\n> explanation why it is a safe thing to do.\n\nRemoving the 1 _may_ be a safe thing to do, because the value\nfragment->patch_method\nis finally used in a switch statement, which makes it apparent that\nthe difference in the values\nis actually necessary, and maybe not the actual value they hold (Also,\nrandom but distinct\nvalues still pass the test cases).\n\n> > static void free_fragment_list(struct fragment *list)\n> > {\n> > @@ -918,14 +920,17 @@ 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> > +enum diff_name {\n> > + DIFF_OLD_NAME = 0,\n> > + DIFF_NEW_NAME\n> > +};\n>\n> Ditto.\n\nI think that since enums start with zero by default, you are right in\nsaying that the '=0' here can be removed.\nI will do so.\n\n> > static int gitdiff_verify_name(struct gitdiff_data *state,\n> > const char *line,\n> > int isnull,\n> > char **name,\n> > - int side)\n> > + enum diff_name side)\n> > {\n> > if (!*name && !isnull) {\n> > *name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);\n> > @@ -1910,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,\n> > int llen, used;\n> > unsigned long size = *sz_p;\n> > char *buffer = *buf_p;\n> > - int patch_method;\n> > + enum binary_type_deflated patch_method;\n>\n> This is not quite sufficient to achieve the goal of helping \"modern\n> debuggers\" that was stated in the proposed log message, is it?\n> parse_binary_hunk() copies the value from this local variable to a\n> member in the fragment being parsed, like so:\n>\n> frag->binary_patch_method = patch_method;\n>\n> but the thing is, as we have seen earlier, a compiler macro is used\n> to (ab)use the \"leading\" member and call it \"binary_patch_method\".\n> The type of that member is \"unsigned long\".\n>\n> Now if our ultimate goal were to use enum instead of macro, then an\n> obvious \"solution\" would be to stop abusing \"leading\". Instead, you\n> would add \"enum binary_type_deflated binary_patch_method\" member to\n> the fragment struct and use the enum throughout.\n\nIt does appear to be so. Introducing a new enum binary_type_deflated\nbinary_patch_method inside of\nstruct fragment would indeed be a solution.\nThis could be plausible, because struct fragment does not appear outside of\napply.c. Your suggestion appears to be the only right way to do it, so\nas to achieve the\ngoal of this patch series.\n\n> But is it worth it?\n>\n> Using enum instead of macro is *NOT* a goal. If doing so makes our\n> code better, we may do so---which tells us that use of enum is not a\n> goal but a means to make our code better. Is adding an extra member\n> that is used only for binary patches improve our code? I dunno.\n\napply.c actually seemed to be the best place to start such a change, because the\nmacros in the file suit such a change the best for a start. It would\nhave brought\nunnecessary overhead in many other files, specifically those which\nwould affect a _lot_ of\nthe source code. It is just that the changes appear contained enough\nto not cause a lot of\nnoise.\nYour suggestions are very correct, I will make sure to keep them in\nmind in the future.\n\nThanks a lot!\nVinayak\n"},{"id":"472205","messageId":"xmqqttzlr0wm.fsf@gitster.g","threadId":"59245","inReplyTo":"CADE8Nao=ZrF7_0+G=jB8=j5Aji4ndCLfMp-xjbSpc1HKmmFM7w@mail.gmail.com","subject":"Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-16T17:11:05Z","receivedAt":"2023-02-16T17:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vinayak Dev writes:\n\n>> As a faithful rewrite, forcing the same values to be given as before\n>> by saying that \"_DEFLATED must be 1\" is a good thing to do, but once\n>> the dust settled from the patch, it would be a good idea to go back\n>> to the code and see if the values MUST be these, or if it is fine to\n>> use any value as long as they are distinct. If it is the latter,\n>> then it would make a good follow-up patch to remove \"= 1\", with an\n>> explanation why it is a safe thing to do.\n>\n> Removing the 1 _may_ be a safe thing to do, because ...\n\nI didn't mean to suggest you think about it _NOW_ in the context of\nworking on this patch. Rather the opposite. Let's have a faithful\nrewrite first and then as a follow-on work after this patch becomes\npart of \"git\", a further clean-up like that can be a good idea.\n\n>> > +enum diff_name {\n>> > + DIFF_OLD_NAME = 0,\n>> > + DIFF_NEW_NAME\n>> > +};\n>>\n>> Ditto.\n>\n> I think that since enums start with zero by default, you are right in\n> saying that the '=0' here can be removed.\n\nNot what I meant. I was referring to the lack of trailing comma.\n\n> I will do so.\n\nPlease don't.\n\nThanks.\n"},{"id":"472206","messageId":"CADE8NaqxVZ3oA2rg6ySR6L-t+b9bM07YTKtaDZ3u=N6U_t_T2Q@mail.gmail.com","threadId":"59245","inReplyTo":"xmqqttzlr0wm.fsf@gitster.g","subject":"Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-16T17:21:05Z","receivedAt":"2023-02-16T17:21:23Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"On Thu, 16 Feb 2023 at 22:41, Junio C Hamano wrote:\n>\n> Vinayak Dev writes:\n>\n> >> As a faithful rewrite, forcing the same values to be given as before\n> >> by saying that \"_DEFLATED must be 1\" is a good thing to do, but once\n> >> the dust settled from the patch, it would be a good idea to go back\n> >> to the code and see if the values MUST be these, or if it is fine to\n> >> use any value as long as they are distinct. If it is the latter,\n> >> then it would make a good follow-up patch to remove \"= 1\", with an\n> >> explanation why it is a safe thing to do.\n> >\n> > Removing the 1 _may_ be a safe thing to do, because ...\n>\n> I didn't mean to suggest you think about it _NOW_ in the context of\n> working on this patch. Rather the opposite. Let's have a faithful\n> rewrite first and then as a follow-on work after this patch becomes\n> part of \"git\", a further clean-up like that can be a good idea.\n\nI am sorry, I just got a little too excited! I will follow-up at the\nright time, I assure you.\n\n> >> > +enum diff_name {\n> >> > + DIFF_OLD_NAME = 0,\n> >> > + DIFF_NEW_NAME\n> >> > +};\n> >>\n> >> Ditto.\n> >\n> > I think that since enums start with zero by default, you are right in\n> > saying that the '=0' here can be removed.\n>\n> Not what I meant. I was referring to the lack of trailing comma.\n\nOk! Understood.\n\n> > I will do so.\n>\n> Please don't.\n\nOk! I won't. :)\n\nThanks a lot for your reply!\nVinayak\n"}]}