Arnav Bhate <bhatearnav@xxxxxxxxx> writes:
[snip]
>> While this is correct, now we have 'i' & 'j' as iteration variables,
>> generally this is used in O(n^2) loops to define the outer and inner
>> loops. Here, however, we use it to simply define two different types. I
>> find this deviation from convention a little confusing.
>>
>> Perhaps, we could simply utilize the option of intializing loop
>> variables in the loop itself?
>>
>> diff --git a/pathspec.c b/pathspec.c
>> index 89663645e1..ff8854afb8 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const
>> struct pathspec *pathspec,
>> char *seen,
>> enum ps_skip_worktree_action sw_action)
>> {
>> - int num_unmatched = 0, i;
>> + int num_unmatched = 0;
>>
>> /*
>> * Since we are walking the index as if we were walking the directory,
>> @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const
>> struct pathspec *pathspec,
>> * mistakenly think that the user gave a pathspec that did not match
>> * anything.
>> */
>> - for (i = 0; i < pathspec->nr; i++)
>> + for (int i = 0; i < pathspec->nr; i++)
>> if (!seen[i])
>> num_unmatched++;
>> if (!num_unmatched)
>> return;
>> - for (i = 0; i < istate->cache_nr; i++) {
>> + for (unsigned int i = 0; i < istate->cache_nr; i++) {
>> const struct cache_entry *ce = istate->cache[i];
>> if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>> (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
>>
>> This would read much cleaner and also avoid two different loop
>> variables. WDYT?
>
> We could certainly do that. My impression was that the convention was
> not to do so.
>
This is specifically mentioned in 'Documentation/CodingGuidelines':
since late 2021 with 44ba10d6, we have had variables declared in the
for loop "for (int i = 0; i < 10; i++)".
So actually, the convention is to do so.
>> Also a bigger question is, shouldn't the type of `pathspec.nr` and
>> 'istate.cache_nr' be the actual change required? Shouldn't they be set
>> to 'size_t'?
>
> I tried that first and found that it required making a large number of
> changes spread over many files. As noted in my commit message, both
> signed and unsigned types are used at different places for this
> purpose.
>
I can see that, but that is the correct change, no? either ways, it
should be called out in the commit message why that was not the approach
taken.
My personal take is that this fix is more of a bandaid, it would be
better to fix the issue at source. Adding these smaller local fixes is
going in the wrong direction, because we're increasing touchpoints which
have to be changed when the actual fix is made.
>>> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
>>> {
>>> struct index_state *istate = the_repository->index;
>>> char *seen = xcalloc(pathspec->nr, 1);
>>> - int i;
>>> + unsigned int i;
>>>
>>
>> Nit: We could also drop this and move the initialization to the line
>> below.
>>
>>> for (i = 0; i < istate->cache_nr; i++) {
>>> struct cache_entry *ce = istate->cache[i];
>>> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
>>> if (element[1] != '(') {
>>> /* Process an element in shorthand form (e.g. ":!/<match>") */
>>> strbuf_addstr(sb, ":(");
>>> - for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>> + for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>> if ((magic & pathspec_magic[i].bit) &&
>>> pathspec_magic[i].mnemonic) {
>>> if (sb->buf[sb->len - 1] != '(')
>>
>> Shouldn't we use 'size_t' for this, since we're iterating over the
>> elements of an array?
>
> We can use size_t there.
>
>>
>>> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>>
>>> for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
>>> size_t len = strcspn_escaped(pos, ",)");
>>> - int i;
>>> + unsigned int i;
>>>
>>
>> This too should be 'size_t'.
>>
>>> if (pos[len] == ',')
>>> nextat = pos + len + 1; /* handle ',' */
>>> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>> if (starts_with(pos, "prefix:")) {
>>> char *endptr;
>>> *prefix_len = strtol(pos + 7, &endptr, 10);
>>> - if (endptr - pos != len)
>>> + if ((size_t)(endptr - pos) != len)
>>> die(_("invalid parameter for pathspec magic 'prefix'"));
>>> continue;
>>> }
>>
>> This makes sense. But is it guaranteed that `endptr - pos` is greater
>> than 0?
>
> endptr - pos will be greater than or equal to zero, as endptr is set by
> strtol
>
>>> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>>>
>>> for (pos = elem + 1; *pos && *pos != ':'; pos++) {
>>> char ch = *pos;
>>> - int i;
>>> + unsigned int i;
>>>
>>
>> This too, should be 'size_t'
>>
>>> /* Special case alias for '!' */
>>> if (ch == '^') {
>>> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
>>>
>>> void pathspec_magic_names(unsigned magic, struct strbuf *out)
>>> {
>>> - int i;
>>> + unsigned int i;
>>
>> This can be inlined and made 'size_t'.
>>
>>> for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>> const struct pathspec_magic *m = pathspec_magic + i;
>>> if (!(magic & m->bit))
>>> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,
>>> int pathspec_needs_expanded_index(struct index_state *istate,
>>> const struct pathspec *pathspec)
>>> {
>>> - unsigned int i, pos;
>>> - int res = 0;
>>> + unsigned int pos;
>>> + int i, res = 0;
>>> char *skip_worktree_seen = NULL;
>>>
>>
>> This can be inlined, but this change is done to match 'pathspec.nr''s
>> type. This goes to my earlier question, I would say we first need to
>> modify 'pathspec.nr' itself to be 'size_t'.
>>
>>> /*
>>> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>>> * - not-in-cone/bar*: may need expanded index
>>> * - **.c: may need expanded index
>>> */
>>> - if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
>>> + if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
>>> path_in_cone_mode_sparse_checkout(item.original, istate))
>>> continue;
>>>
>>
>> Similar here, I see the types of 'item.len' and 'item.nowwildcard_len'
>> are 'int'. Do they need to be 'size_t'?
>
> Same as above, will require a large number of changes.
>
Continuing from top, I would say that the goal is not to simply remove
all 'DISABLE_SIGN_COMPARE_WARNINGS' definitions in the easiest way
possible. Specially when we end up adding more things to be fixed
eventually.
I would say perhaps picking one of these structs and fixing their types
might be more suited. Let's see what others have to say here!
Attachment:
signature.asc
Description: PGP signature
