From: Philippe Blain Date: Fri, 07 Jan 2022 13:27:00 GMT Subject: Re: [PATCH] submodule.h: use a named enum for RECURSE_SUBMODULES_* Message-ID: <3160705c-d95c-c389-e929-f1f6bbcd80fc@gmail.com> In-Reply-To: Hi Junio, Le 2022-01-05 à 16:20, Junio C Hamano a écrit : > "Philippe Blain via GitGitGadget" writes: > >> From: Philippe Blain >> >> Using a named enum allows casting an integer to the enum type in both >> GDB and LLDB: >> >> (gdb) p (enum diff_submodule_format) options->submodule_format >> $1 = DIFF_SUBMODULE_LOG > > Hmph, this was somewhat unexpected and quite disappointing. > > Because that's not what those "Let's move away from #define'd > constants and use more enums" said when they sold their "enum" to > us. In the diff_options struct, the .submodule_format member is of > type enum already, so, if we trust what they told us when they sold > their enums, "p options->submodule_format" should be enough to give > us "DIFF_SUBMODULE_LOG", not "1", as its output. Do you really need > the cast in the above example? Yes, you are right that my example does not reflect what I'm saying, since options->submodule_format is not an int. I checked and indeed we do not need any cast to get "DIFF_SUBMODULE_LOG". We do need it when dealing with int's, which is not the case here. I'll try to find a better example. > >> Name the enum listing the different RECURSE_SUBMODULES_* modes, to make >> debugging easier. > > Even though this patch may be a good single step in the right > direction, until it is _used_ to declare a variable or a member of a > struct of that enum type, it probably is not useful as much as it > could be. The user needs to know which enum is stored in that "int" > variable or member the user is interested in to cast it to. Yes, that's true. But when I came across that, I was in a place of the code where some int was compared with a constant in this enum, RECURSE_SUBMODULES_something. So it would have been easy to check where the enum is declared, learn its name and use it to cast the int to the enum type. That's the kind of situation I have in mind. > > That is, many changes like this one. > > diff --git i/builtin/pull.c w/builtin/pull.c > index c8457619d8..f2fd4784df 100644 > --- i/builtin/pull.c > +++ w/builtin/pull.c > @@ -71,7 +71,7 @@ static const char * const pull_usage[] = { > /* Shared options */ > static int opt_verbosity; > static char *opt_progress; > -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > +static enum submodule_recurse_mode recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > > /* Options passed to git-merge or git-rebase */ > static enum rebase_type opt_rebase = -1; > Yes, this is a parallel effort that could be done, I agree, but my patch was meant to help in the mean time. Thanks, Philippe.