Julian Foad wrote:
> So, do you see any of these signed/unsigned mismatch warnings from a
> Subversion build on Win32?
Some, a few not in our code. Mostly I see narrowing warnings, e.g.,
apr_off_t to apr_size_t (64->32 bits on Win32), which I've not yet
figured out how to fix cleanly (they're mostly in the diff code and will
only bother someone who has text files larger that 4G, so they aren't
very high on my priority list...).
> How many?
I don't recall at this moment, I don't have a build box handy.
> Do they include this assignment introduced by r11162 in
> subversion/mod_dav_svn/log.c line 244?
> unsigned int limit;
> limit = atoi(child->first_cdata.first->text);
Ah, but in this case, we're converting from int to unsigned int, which
in C, is a standard integer promotion and I'd be very surprised if any
compiler warned about it.
However, there's a more fundamental bug in that line, namely the use of
atoi. atoi("fubar") will happily return 0, and should therefore never be
used to parse program options. We should use strtol here, check that it
converted the whole string, check that the result is nonnegative (or
positive, --limit=0 is a bit weird) and is less than UINT_MAX, then we
can safely cast it.
> "gcc -W" catches signed/unsigned comparisons, of which we have three
> "unsigned >= 0" tautologies and no variable/variable mismatches
> (except for one which I introduced recently in a test file and will
> fix by changing the local variable to unsigned int).
>
> "gcc -Wconversion" catches signed/unsigned conversions as well as
> other conversions (e.g. width). We currently trigger 240 in total,
> 106 width changes and 134 signed/unsigned conversions.
>
> I haven't found an option that generates warnings for mismatched
> assignments, like that "limit = atoi()".
Quite as expected.
> We have to work with the limitations of the language we use. Unsigned
> integers are great for low-level work, but not for high-level C
> interfaces.
The fact that C has unsigned integers is not a limitation of the
language. What does happen quite often is that people unthinkingly use
(witness that atoi) or design broken APIs that fail to take conversion
edge cases into account. Notice that even changing the limit paramter to
a signed int won't fix the bug in the atoi usage.
However, you make a good case with listing precedents, two of which
(svn_config_enumerate, svn_txdelta_window_t) and one you did not mention
(svn_filesize_t) I wrote myself... Looking back, it strikes me that
limit should be an svn_revnum_t, because it represents the same range of
values.
So... I'm withdrawing my objection. Let's change the thing to signed,
and by all means fix that atoi.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 15 10:24:57 2004