Fernando Gouveia Lima fernandolimabusiness@xxxxxxxxx writes:
> @@ -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.
> @@ -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/
