Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > On Tue, Sep 28 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> >>> Use the "enum parse_opt_flags" instead of an "int flags" as arguments >>> to the various functions in parse-options.c. This will help to catch >>> cases where we're not handling cases in switch statements, and ... > But I think this is perfectly good use of enums, we use this > enums-as-bitfields pattern in various other places, > e.g. builtin/rebase.c's "flags", the "commit_graph_write_flags", > "expire_reflog_flags" & "todo_item_flags", just to name a few from some > quick grepping. Many codepaths already misusing is not an excuse to add another ;-) But ... > One advantage is that integrates nicely with some wider C > tooling. E.g. before this series, starting "git stash show" under gdb > and inspecting flags: > > (gdb) p flags > $1 = 9 > > And after: > > (gdb) p flags > $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN) ... this is a pleasant surprise---the last time I checked, debuggers were that clever to notice that the distinct values are names for individual bits. I can buy this as an argument for using enums for names for individual bits. For this to work, obviously, variable and struct members need to be given the appropriate type. So I agree with the change in 2/10. Except that one place there was a change related to a different enum that is a true enumeration in this step. It belongs to 3/10, I think. Also, the sales pitch for this step in the proposed commit log message needs rewriting---this will "not" help to catch cases where we're not handling cases in switch statements; if you are selling it because you think it will help debuggers and other tooling, let's describe it as such. Even though I think debuggers are overrated ;-) Thanks.
