* [PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c
@ 2025-05-20 19:18 Lucas Eiji
2025-05-20 21:10 ` Junio C Hamano
0 siblings, 1 reply; 2+ messages in thread
From: Lucas Eiji @ 2025-05-20 19:18 UTC (permalink / raw)
To: git; +Cc: Eiji Uchiyama, Lucas Eiji Uchiyama
From: Eiji Uchiyama <eijiuchiyama@github•com>
This is an initial contribution to git, based on the SoC 2025 ideas
for microprojects. It removes the DISABLE_SIGN_COMPARE_WARNINGS macro and
solves the warnings generated by running make DEVELOPER=1 -j4
Signed-off-by: Lucas Eiji Uchiyama <eijiuchiyama@usp•br>
---
add-interactive.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/add-interactive.c b/add-interactive.c
index 97ff35b6f1..3a0c44c47f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1,5 +1,3 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
#include "git-compat-util.h"
#include "add-interactive.h"
#include "color.h"
@@ -213,10 +211,10 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
else if (index > 0 &&
starts_with(list->sorted.items[index - 1].string, string))
return -1;
- else if (index + 1 < list->sorted.nr &&
+ else if (index + 1 < (long int)(list->sorted.nr) &&
starts_with(list->sorted.items[index + 1].string, string))
return -1;
- else if (index < list->sorted.nr &&
+ else if (index < (long int)(list->sorted.nr) &&
starts_with(list->sorted.items[index].string, string))
item = list->sorted.items[index].util;
else
@@ -244,7 +242,7 @@ static void list(struct add_i_state *s, struct string_list *list, int *selected,
color_fprintf_ln(stdout, s->header_color,
"%s", opts->header);
- for (i = 0; i < list->nr; i++) {
+ for (i = 0; i < (long int)(list->nr); i++) {
opts->print_item(i, selected ? selected[i] : 0, list->items + i,
opts->print_item_data);
@@ -385,7 +383,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
to = from + 1;
}
- if (from < 0 || from >= items->items.nr ||
+ if (from < 0 || from >= (long int)(items->items.nr) ||
(singleton && from + 1 != to)) {
color_fprintf_ln(stderr, s->error_color,
_("Huh (%s)?"), p);
@@ -395,7 +393,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
break;
}
- if (to > items->items.nr)
+ if (to > (long int)(items->items.nr))
to = items->items.nr;
for (; from < to; from++)
@@ -859,7 +857,7 @@ static int get_untracked_files(struct repository *r,
add_pattern_list(&dir, EXC_CMDL, "--exclude option");
fill_directory(&dir, r->index, ps);
- for (i = 0; i < dir.nr; i++) {
+ for (i = 0; (long int)(i) < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
if (index_name_is_other(r->index, ent->name, ent->len)) {
@@ -939,7 +937,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
return -1;
if (unmerged_count || binary_count) {
- for (i = j = 0; i < files->items.nr; i++) {
+ for (i = j = 0; i < (long int)(files->items.nr); i++) {
struct file_item *item = files->items.items[i].util;
if (item->index.binary || item->worktree.binary) {
@@ -972,7 +970,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
struct strvec args = STRVEC_INIT;
struct pathspec ps_selected = { 0 };
- for (i = 0; i < files->items.nr; i++)
+ for (i = 0; i < (long int)(files->items.nr); i++)
if (files->selected[i])
strvec_push(&args,
files->items.items[i].string);
@@ -1018,7 +1016,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
oid_to_hex(!is_initial ? &oid :
s->r->hash_algo->empty_tree),
"--", NULL);
- for (i = 0; i < files->items.nr; i++)
+ for (i = 0; i < (long int)(files->items.nr); i++)
if (files->selected[i])
strvec_push(&cmd.args,
files->items.items[i].string);
@@ -1146,7 +1144,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
ssize_t i;
int res = 0;
- for (i = 0; i < ARRAY_SIZE(command_list); i++) {
+ for (i = 0; i < (long int)(ARRAY_SIZE(command_list)); i++) {
struct command_item *util = xcalloc(1, sizeof(*util));
util->command = command_list[i].command;
string_list_append(&commands.items, command_list[i].string)
@@ -1183,7 +1181,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
struct command_item *util;
i = list_and_choose(&s, &commands, &main_loop_opts);
- if (i < 0 || i >= commands.items.nr)
+ if (i < 0 || i >= (long int)(commands.items.nr))
util = NULL;
else
util = commands.items.items[i].util;
--
2.34.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c
2025-05-20 19:18 [PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c Lucas Eiji
@ 2025-05-20 21:10 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2025-05-20 21:10 UTC (permalink / raw)
To: Lucas Eiji; +Cc: git, Eiji Uchiyama, Lucas Eiji Uchiyama
Lucas Eiji <lucaseiji54@gmail•com> writes:
> From: Eiji Uchiyama <eijiuchiyama@github•com>
>
> This is an initial contribution to git, based on the SoC 2025 ideas
> for microprojects. It removes the DISABLE_SIGN_COMPARE_WARNINGS macro and
> solves the warnings generated by running make DEVELOPER=1 -j4
Your first sentence is not something you want to carve in stone as
part of the official history, and should not be part of the proposed
commit log message. Yet, it is very nice of you to tell your
reviewers that you are the first-time contributor and may want extra
help by community members. If you want to do so, do it below the
three-dash line that is between the proposed log message and the
diffstat.
The usual way to compose a log message of this project is to
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
in this order. I would expect to see something along the lines of ...
Subject: [PATCH] add-interactive.c: squelch -Wsign-compare warnings
A handful of functions in add-interactive.c compare .nr member
of a string_list structure (which is of type size_t) with a
local variable (which often is of type int), and triggers
compiler warnings due to -Wsign-compare being part of the
developer configuration.
Squelch them by DOING THIS AND THAT.
... but I'll refrain from filling the "DOING THIS AND THAT" part.
> - else if (index + 1 < list->sorted.nr &&
> + else if (index + 1 < (long int)(list->sorted.nr) &&
Well, by sprinkling casts all over the place, you can squelch almost
any compiler warnings, but the real question you should ask is: is
it making the code more correct, or at least not worse?
For example, what does the above code do on a platform where size_t
is 64-bit unsigned integer, and "long int" is 32-bit signed integer?
For those who are reading from sidelines, "index" here is "int".
Very locally on this line, I think the more correct fix may be to
declare that "index" is of type "size_t" (not "int"). We may also
have to barf when "index + 1" overflows "size_t".
But do not go there yet.
I think the real culprit is that string_list is misdesigned in that
most of the code there work with platform natural "int" type
(e.g. get_entry_index() that looks for the location in the array for
a given string does bisection using "int", add_entry() that returns
where in the array of strings it inserted the new one using "int",
string_list_find_insert_index() that finds an existing entry or the
location a new entry should be inserted into uses "int"), yet it
declares the size of the array of the string using "size_t".
Those index-yielding API functions (and internal implementation
details) in string_list should be using "size_t" to express where in
the array they want to point at, or "int" that may be a lot shorter
(and has only half the range of "unsigned" on the positive side)
would never be adequate. Or change the .nr member to "int" (of
course, the code that grows the array must be careful not to
overflow .nr and let it wraparound---but the code must be careful no
matter what type it is; declaring "size_t nr;" alone does not fix
anything).
The string_list API must be fixed first before fixing the calling
programs like this one, I would think.
I'll stop here, as all the other changes to this file were due to
the misdesign of the string_list API.
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/
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-05-20 21:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-20 19:18 [PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c Lucas Eiji
2025-05-20 21:10 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox