Hi Junio,
Le 2022-01-05 à 16:20, Junio C Hamano a écrit :
"Philippe Blain via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
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.
