Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum
On Wed, 15 Feb 2023 at 23:19, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> Vinayak Dev <vinayakdev.sci@gmail.com> writes:
>
> > diff --git a/apply.c b/apply.c
> > index 5cc5479c9c..b2a03d9fc3 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -205,8 +205,10 @@ struct fragment {
> > * or deflated "literal".
> > */
> > #define binary_patch_method leading
>
> Notice and explain what this line is doing, perhaps?It does appear that the macro binary_patch_method actually refers to an unsigned long. I think leading represents the number of leading context lines in a hunk/fragment, and the variable is reused to store the type for a binary fragment.
Show 11 quoted lines
> > -#define BINARY_DELTA_DEFLATED 1
> > -#define BINARY_LITERAL_DEFLATED 2
> > +enum binary_type_deflated {
> > + BINARY_DELTA_DEFLATED = 1,
> > + BINARY_LITERAL_DEFLATED
> > +};
>
> These days, we not just allow but encourage enum definitions to have
> a trailing comma after the last item, UNLESS we want to signal that
> the one at the end right now MUST stay to be the last one (e.g. a
> sentinel at the end).> A patch that adds a new item to, removes an existing item from, or
> shuffles existing items in the list can be free of unnecessary patch
> noise to omit the last comma.
Show 7 quoted lines
> As a faithful rewrite, forcing the same values to be given as before
> by saying that "_DEFLATED must be 1" is a good thing to do, but once
> the dust settled from the patch, it would be a good idea to go back
> to the code and see if the values MUST be these, or if it is fine to
> use any value as long as they are distinct. If it is the latter,
> then it would make a good follow-up patch to remove "= 1", with an
> explanation why it is a safe thing to do.
Removing the 1 _may_ be a safe thing to do, because the value fragment->patch_method is finally used in a switch statement, which makes it apparent that the difference in the values is actually necessary, and maybe not the actual value they hold (Also, random but distinct values still pass the test cases).
Show 15 quoted lines
> > static void free_fragment_list(struct fragment *list)
> > {
> > @@ -918,14 +920,17 @@ 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
> > +
> > +enum diff_name {
> > + DIFF_OLD_NAME = 0,
> > + DIFF_NEW_NAME
> > +};
>
> Ditto.I think that since enums start with zero by default, you are right in saying that the '=0' here can be removed. I will do so.
Show 31 quoted lines
> > static int gitdiff_verify_name(struct gitdiff_data *state,
> > const char *line,
> > int isnull,
> > char **name,
> > - int side)
> > + enum diff_name side)
> > {
> > if (!*name && !isnull) {
> > *name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);
> > @@ -1910,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
> > int llen, used;
> > unsigned long size = *sz_p;
> > char *buffer = *buf_p;
> > - int patch_method;
> > + enum binary_type_deflated patch_method;
>
> This is not quite sufficient to achieve the goal of helping "modern
> debuggers" that was stated in the proposed log message, is it?
> parse_binary_hunk() copies the value from this local variable to a
> member in the fragment being parsed, like so:
>
> frag->binary_patch_method = patch_method;
>
> but the thing is, as we have seen earlier, a compiler macro is used
> to (ab)use the "leading" member and call it "binary_patch_method".
> The type of that member is "unsigned long".
>
> Now if our ultimate goal were to use enum instead of macro, then an
> obvious "solution" would be to stop abusing "leading". Instead, you
> would add "enum binary_type_deflated binary_patch_method" member to
> the fragment struct and use the enum throughout.It does appear to be so. Introducing a new enum binary_type_deflated binary_patch_method inside of struct fragment would indeed be a solution. This could be plausible, because struct fragment does not appear outside of apply.c. Your suggestion appears to be the only right way to do it, so as to achieve the goal of this patch series.
Show 6 quoted lines
> But is it worth it?
>
> Using enum instead of macro is *NOT* a goal. If doing so makes our
> code better, we may do so---which tells us that use of enum is not a
> goal but a means to make our code better. Is adding an extra member
> that is used only for binary patches improve our code? I dunno.
apply.c actually seemed to be the best place to start such a change, because the macros in the file suit such a change the best for a start. It would have brought unnecessary overhead in many other files, specifically those which would affect a _lot_ of the source code. It is just that the changes appear contained enough to not cause a lot of noise. Your suggestions are very correct, I will make sure to keep them in mind in the future.
Thanks a lot! Vinayak
Vinayak Dev