Re: [Nit] Lots of enumerated type warnings

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2018, 23:44 UTC
Message-ID: <xmqqy3kpihm3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <001501d393d3$b82150d0$2863f270$@nexbridge.com>

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

> Here are a few examples, there are more:
>
> auto_crlf = git_config_bool(var, value);
>   		          ^

The carets in your message do not align to what I think they are trying to point at, but I think the above is pointing at the '=' and wants to say "auto_crlf variable is enum, it gets assigned an integer returned from git_config_bool(), and I do not like that assignment".

For this one I tend to agree with the compiler, meaning that it is ugly to define "enum auto_crlf" in such a way that the first two values happen to match what a logically different "enum" (which is "boolean") assigns the two values to. And this judgment does not change whether git_config_bool() actually returns an enum or an int (the code in reality returns the latter).

I do not think people would terribly mind a patch to turn the above into:

  auto_crlf = git_config_bool(var, value) ? AUTO_CRLF_FALSE : AUTO_CRLF_TRUE;

Show 6 quoted lines

> "/home/jenkins/.jenkins/workspace/Build_Git/config.c", line 1147:
> warning(272): 
>           enumerated type mixed with another type
>
> type = sha1_object_info(s->oid.hash, &s->size);
>   			     ^

/* returns enum object_type or negative */ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)

The function has been like this forever, I suspect, and I would say "this gives negative when error, or enum we know is non-negative" is quite a reasonable thing to do, but the enum has OBJ_BAD defined to be negative, so probably it is more kosher if sha1_object_info() is declared to return "enum object_type" instead of int.

Show 6 quoted lines

> "/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 3618:
> warning(272): 
>           enumerated type mixed with another type
>
> options->color_moved = diff_color_moved_default;
>   	                     ^

If color_moved field is declared to be an enum, the _default variable should be, too. I do not think it is such a controversial fix.