Lucas Eiji <lucaseiji54@xxxxxxxxx> writes:
> From: Eiji Uchiyama <eijiuchiyama@xxxxxxxxxx>
>
> 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/
