On 12.03.2015 09:27, Branko Čibej wrote:
> On 12.03.2015 08:49, Bert Huijben wrote:
>> This resolves several signed compared to unsigned warnings on Windows.
>>
>> It is nice to fix warnings for some favorite compiler, but there are
>> still > 200 warnings left on Windows, especially on cases that assume
>> int and size_t variants have identical length, which doesn’t hold on
>> Win64.
>
> Our policy is to try to compile without warnings in maintainer mode on
> most platforms. I realize that Windows tends to be an exception
> (although, to be fair, many of this class of warnings in the Windows
> build are false positives).
>
> But the point is that your change added a new warning about a
> narrowing assignment; in other words, there must be a better way to
> both resolved the signed/unsigned warnings you see, and the narrowing
> conversion warning that I see; which, by the way, *is* important on
> both Win32 and Win64, since we're now assigning an unsigned value to a
> signed one, even if it's of the same size, so overflow could happen in
> theory.
>
> For example, if you change the type of context_size in
> svn_diff3__file_output_baton_t, you probably have to change it in
> context_saver_t as well. Both structs are local to diff_file.c (which
> makes me wonder while svn_diff3__file_output_baton_t has that
> svn_diff3__ prefix in the first place). Assuming that using an
> unsigned value there is even correct.
Looking at the code some more, it turns out that context_size is an int
in lots of places: svn_diff_options_t, svn_diff__file_output_baton_t,
context_saver_t, many function parameters in that file, etc. The code
almost invariably expects a signed value. In other words, changing the
type only in svn_diff3__file_output_baton_t is simply wrong. At first
glance it appears that what you really need to do is tweak the types of
the other fields in context_saver_t, but at the moment I'm on the road
and not in a position to prove that. In general, however, when fixing a
warning it's often a good idea to consider the semantics of the
surrounding code, not just make a minimal change to make the compiler
shot up.
-- Brane
Received on 2015-03-12 12:34:36 CET