Turn more preprocessor constants into enums

[dscho]

In Git's source, code, there are a number of constants such as:

#define DT_UNKNOWN      0
#define DT_DIR          1
#define DT_REG          2
#define DT_LNK          3

In recent years, the Git project started preferring enum values instead, for added type safety and to allow stronger reasoning via static analysis.

Even values that are intended to be used in bit fields can benefit from this, e.g.

#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
#define ACTION_GET_REGEXP (1<<2)
#define ACTION_REPLACE_ALL (1<<3)
#define ACTION_ADD (1<<4)
#define ACTION_UNSET (1<<5)
#define ACTION_UNSET_ALL (1<<6)
#define ACTION_RENAME_SECTION (1<<7)
#define ACTION_REMOVE_SECTION (1<<8)
#define ACTION_LIST (1<<9)
#define ACTION_EDIT (1<<10)
#define ACTION_SET (1<<11)
#define ACTION_SET_ALL (1<<12)
#define ACTION_GET_COLOR (1<<13)
#define ACTION_GET_COLORBOOL (1<<14)
#define ACTION_GET_URLMATCH (1<<15)

As there are too many instances of this kind in Git's source code, a good first issue to tackle would be to find a group of such preprocessor constants and turn them into anenum. Also find where those constants are stored in variables and in structs and passed around as function parameters, and change the type of those variables, fields and parameters to the new enum.

Suggested in https://public-inbox.org/git/20190923180649.GA2886@szeder.dev/

[dscho]

Correction: in https://public-inbox.org/git/xmqqsgobg0rv.fsf@gitster-ct.c.googlers.com/, the Git maintainer frowned upon using enums for bit field values. So let's not convert those.

[harry-hov]

we have to convert every single preprocessor constant into enum ? (except bit field values)

[dscho]

we have to convert every single preprocessor constant into enum ?

We don't have to 😀

But it would be nice to make older code consistent with newer coding conventions, no? And when breaking it apart into the individual enums to be created, this kind of project is small enough to allow first-time contributors to "get their feet wet", so to say, i.e. to make themselves familiar with the process and style of Git's code contributions.

[harry-hov]

Looks good issue for me to getting started. 😄

[dscho]

Looks good issue for me to getting started.

Cool!

Do keep in mind that you do not need to do all of them. Just pick one, for a start.

[phil-blain]

Question for lurkers: when compiling with -g3 (GCC) or -fdebug-macro (Clang), debugging symbols for preprocessor constants are generated. Is there an equivalent for enumeration constants ?

[phil-blain]

Answering my own question:

In GDB, if RECURSE_SUBMODULES_OFF is an enum value, p RECURSE_SUBMODULES_OFF just returns RECURSE_SUBMODULES_OFF. This is not very helpful because sometimes variables that are compared with such enums in the Git source code are ints, so you might want to know what integer values the enums values map to. This can be done with any of the following commands:

(gdb) p (int) RECURSE_SUBMODULES_OFF
0
(gdb) p /d RECURSE_SUBMODULES_OFF
0
(gdb) p RECURSE_SUBMODULES_OFF+0
0

Source: https://sourceware.org/bugzilla/show_bug.cgi?id=11067

In LLDB (tested in version 10.0.0), it seems enums have to be named for the debugger to understand the enum constants, i.e. with this patch:

diff --git i/submodule.h w/submodule.h
index 84640c49c1..61151810cb 100644
--- i/submodule.h
+++ w/submodule.h
@@ -13,7 +13,7 @@ struct repository;
 struct string_list;
 struct strbuf;
 
-enum {
+enum submodule_recurse_mode {
        RECURSE_SUBMODULES_ONLY = -5,
        RECURSE_SUBMODULES_CHECK = -4,
        RECURSE_SUBMODULES_ERROR = -3,

you can do

(lldb) p (int) submodule_recurse_mode::RECURSE_SUBMODULES_ON_DEMAND
(int) $0 = -1
(lldb) p submodule_recurse_mode::RECURSE_SUBMODULES_ON_DEMAND+0
(int) $1 = -1

---

You can also check in the other direction, i.e. cast the int to the enum type, if the enum is a named enum:
With the same patch as above:

In GDB you can do:

(gdb) p (enum submodule_recurse_mode) recurse_submodules
$1 = RECURSE_SUBMODULES_ON

In LLDB you can do:

(lldb) p (submodule_recurse_mode) recurse_submodules
(submodule_recurse_mode) $1 = RECURSE_SUBMODULES_ON
(lldb) p (enum submodule_recurse_mode) recurse_submodules
(submodule_recurse_mode) $2 = RECURSE_SUBMODULES_ON

---

EDIT I've added these tips to my "Debugging Git" Gist:

• https://gist.github.com/phil-blain/17c67740bd26e66f4851fe0c07230ea4#including-preprocessor-macros
• https://gist.github.com/phil-blain/17c67740bd26e66f4851fe0c07230ea4#enums

[ffyuanda]

Is this issue still doable? I kinda want to know git contribution better so I'm wondering if I should take a shot at this one?

[dscho]

@ffyuanda I would actually recommend thinking of something you want to change in Git. If there has been any moment where you said "if Git only would...", that's a good idea to pursue. Turning preprocessor constants into enums is not only boring, it also has been met with lackluster enthusiasm by the Git maintainer.