Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning
Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:
Show 9 quoted lines
> @@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,
> int flags UNUSED,
> void *cb_data)
> {
> - int i;
> + long unsigned int i;
> struct object *obj;
> enum object_type objtype;
> enum decoration_type deco_type = DECORATION_NONE;The complaint is about this line of code:
for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {where ref_namespace array is of NAMESPACE__COUNT size, which is the last enum in "enum ref_namespace". Anybody can tell it is a small integer (like 9) that would fit comfotably well within the platform natural "int" on anybody that can run Git.
ARRAY_SIZE() is defined in turns of sizeof(), so its type is "size_t"; it cannot be narrower than "int", but that is immaterial. Comparing 'i' that counts up from 0 will never cause a problem.
And the compiler ought to know all of the above, or should stay silent.
The compiler is wrong in this case. As I already have said on this list a few times, -Wsign-compare is often garbage and we should not bend over backwards to butcher perfectly good code only to silence its false positives.
Show 7 quoted lines
> @@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,
> }
> strbuf_addf(filename, "%04d-%s", nr, subject);
>
> - if (max_len < filename->len)
> + if (max_len < (int) filename->len)
> strbuf_setlen(filename, max_len);
This conversion is wrong, even if your compiler is made silent with this change. The function begins like this:
void fmt_output_subject(struct strbuf *filename,
const char *subject,
struct rev_info *info)
{
const char *suffix = info->patch_suffix;
int nr = info->nr;
int start_len = filename->len;
int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);The filename variable is a pointer to a strbuf, whose .len member is of type size_t, which can be wider than a platform natural "int". Assigning it to "int start_len" risks overflowing and wrapping around, and "int max_len" is also wrong.
So casting a large filename->len that would not fit within platform natural "int" may make your compiler silent when you compare it with max_len that is also platform natural "int". But at that point, what are you comparing with what? Imagine on a platform with 32-bit int and when the actual value of filename->len is very very long and longer than say 3 billion bytes.
In this function, I think a more reasonable thing to do is to express various length invoved in size_t, and make sure computations among them does not over/underflow.
Side note: Those who are maintaining GSoC microproject ideas
page. This is the second time in a few days that I had to
look at a patch that makes the code worse by blindly trying
to squelch the -Wsign-compare warnings. Can you please take
that out of the list of suggested tasks? Thanks.
Fernando, please do not get discouraged by _our_ code being sloppy and GSoC microproject ideas page being under-specified. Neither is your fault.
And welcome to Git development community.
[Footnote]
Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we did not add it to the developer settings. A more effective way to squelch them is not by sprinkling the casts like this, but to remove it from config.mak.dev ;-)
https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
