Re: svn commit: r1872108 - /subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c

[Branko Čibej <br...@apache.org>]

On 30.12.2019 13:24, julianfoad@apache.org wrote:
> Author: julianfoad
> Date: Mon Dec 30 12:24:50 2019
> New Revision: 1872108
>
> URL: http://svn.apache.org/viewvc?rev=1872108&view=rev
> Log:
> Avoid integer conversion warning. A follow-up to r1872107.
>
> * subversion/tests/libsvn_subr/mergeinfo-test.c
>   (rangelist_to_array): Add explicit casts.
>
> Modified:
>     subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c?rev=1872108&r1=1872107&r2=1872108&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c Mon Dec 30 12:24:50 2019
> @@ -1841,7 +1841,7 @@ rangelist_to_array(rl_array_t *a,
>        svn_merge_range_t *range = APR_ARRAY_IDX(rl, i, svn_merge_range_t *);
>        int r;
>  
> -      for (r = range->start + 1; r <= range->end; r++)
> +      for (r = (int)range->start + 1; r <= (int)range->end; r++)
>          {
>            a->root[r] = TRUE;
>            a->inherit[r] = range->inheritable;


I really don't like this kind if papering over warnings with typecasts.
Instead, change the type of 'r' to match the types of 'range->start' and
'range->end'.

Yes I know this is just a test helper function.

-- Brane

Re: svn commit: r1872108 - /subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c

[Julian Foad <ju...@apache.org>]

Branko Čibej wrote:
>> -      for (r = range->start + 1; r <= range->end; r++)
>> +      for (r = (int)range->start + 1; r <= (int)range->end; r++)
> 
> I really don't like this kind if papering over warnings with typecasts.
> Instead, change the type of 'r' to match the types of 'range->start' and
> 'range->end'.

Can do.  In fact that was the first thing I did.  I do agree in general 
casts are the worst way.  Then having changed the loop variable type 
here, I looked at the other instances nearby where I use a revision 
number to subscript this array:

rangelist_array_union(rl_array_t *ma, ...)
{
   int r;

   for (r = 0; r <= RANGELIST_TESTS_MAX_REV; r++) {
     ma->root[r] = ...
}

rangelist_array_equal(const rl_array_t *ba, ...)
{
   int r;

   for (r = 0; r <= RANGELIST_TESTS_MAX_REV; r++) {
     if (ba->root[r] ...
}

and thought then it looked marginally better to keep using 'int' in 
those places (because they're low level operations on the array and 
don't care whether the index represents a revision number), and better 
to keep consistent than to just change the loop variable in one of the 
three functions.  But it makes at least as much sense to me the way you 
suggest, so I'm happy to change it.

http://svn.apache.org/r1872115 .

- Julian