On Fri, Feb 10, 2023 at 6:20 AM Vinayak Dev <vinayakdev.sci@xxxxxxxxx> wrote:
> On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@xxxxxxxxx> 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.
