Branko Čibej wrote:
> Julian Foad wrote:
>> * We really, really should not use "unsigned" in the interface. I
>> recommend reverting r11162 which converted various arguments to unsigned.
>
> I disagree. If passing a negative value makes no sense semantically,
> then the parameter should be unsigned.
The semantics argument is good in principle, but unfortunately in the C
language the risks outweigh the benefits. If signed/unsigned conversions were
strongly checked, then I would agree with you.
John Lakos makes a compelling case in his book "Large Scale C++ Software
Design" (as I mentioned in http://svn.haxx.se/dev/archive-2004-10/0075.shtml).
Also, the Java language designers dropped the idea of unsigned integers, and
made all integers signed, and I believe that their experience with C had a lot
to do with it.
And there is no precedent in our public interfaces. The only places where we
use "unsigned" are "unsigned char". On the other hand, we frequently use plain
"int" for things that are semantically non-negative:
* retry_limit in svn_client_get_simple_prompt_provider (and similar functions).
* Return value of svn_config_enumerate (etc.) (the number of times the callback
was called).
* svn_txdelta_window_t::num_ops/src_ops (number of instructions in this window).
* svn_revnum_t (the excuse of wanting to use -1 as a special value could apply
to "limit" as well).
* etc.
>> Just now I tried "svn log --limit=-5" and it listed all revisions.
>
> Then this is a bug in parsing the value of that option. The parser
> should reject negative values.
Certainly. I agree. I suppose r11162 wasn't intended to fix that, in which
case it was unfair of me to reason as if it was.
>> r11162 did not solve that problem, and indeed it introduced
>> signed/unsigned mismatches. Compilers can warn about signed/unsigned
>> mismatches, but they are a necessary evil in many places so the
>> potential to be warned about them isn't often an actual benefit.
>
> I disagree with this, too. signed/unsigned mismatches can and should be
> avoided by careful attention to detail in the implementation. When there
That r11162 was made and has continued to exist, yet contains such mismatches,
is testament to the fact that, while they can in principle be detected and
avoided, in practice they are not being detected and avoided.
> is absolutely no other possible fix (due to defects in other libraries,
> not bugs in Subversion), then usually it's possible to find a safe way
> to cast the value, and document the reason for the cast.
Yes, certainly.
> So I'd suggest that, in maintainer mode, we turn on these warnings where
> possible. Note that they're already on by default on Win32.
So, do you see any of these signed/unsigned mismatch warnings from a Subversion
build on Win32? How many? 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);
"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()".
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.
- Julian
(For GCC users: "gcc -W" gives other types of warning that are not relevant to
us; one such type that contributes many messages can be switched off by adding
"-Wno-unused-parameter".)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 15 03:23:12 2004