On Fri, 10 Feb 2023 at 16:15, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
Thanks a lot for replying!
> On Thu, Feb 9, 2023 at 12:00 PM Vinayak Dev <vinayakdev.sci@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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
