Daniel Berlin wrote:
> On Tue, 2005-12-13 at 16:42 +0000, Julian Foad wrote:
>
>>>+ /* We support format 1 and 2 simultaneously. */
>>>+ if (format == 1 && SVN_FS_BASE__FORMAT_NUMBER == 2)
>>>+ return SVN_NO_ERROR;
>>
>>It doesn't matter whether the current format is 2: there's no reason for this
>>line to automatically stop supporting format 1 when we bump the current format
>>to 3.
>
> This is not true.
>
> We currently assume we only support one format at a time, everywhere.
> We have no policy that says we will keep code to support old formats
> around, AFAIK.
>
> The only real reason we support format 1 and 2 at the same time in
> anything but the dump and loader is because it's a one line of code to
> do so. It's conceivable that if the changes people want to make (dir
> hashing, etc) go in during 1.4, that we won't actually support tons of
> formats at once, because it's no longer one line of code.
>
> Unless we have a policy around i've missed about supporting multiple
> formats at of course, in which case, the only check we should be doing
> in this function is that the format isn't greater than
> SVN_FS_BASE__FORMAT_NUMBER, and not the *current* check we do that
>
>> if (format != SVN_FS_BASE__FORMAT_NUMBER)
OK, I guess you're right.
>>>+void svn_txdelta_to_svndiff2 (svn_stream_t *output,
>>>+ apr_pool_t *pool,
>>>+ svn_txdelta_window_handler_t *handler,
>>>+ void **handler_baton, unsigned int version);
>>
>>It's better not to use modifiers like "unsigned" in APIs, even though it feels
>>like it's making a useful statement about the valid range of the data, as it
>>tends to do more harm than good. Please use plain "int" here.
>
> Can you please explain why?
> In particular,
> 1. I don't believe specifying the proper range of values does more harm
> than good.
> 2. It *is* making a useful statement about the valid range of data.
Here is what I wrote about it before:
http://svn.haxx.se/dev/archive-2004-10/0075.shtml
The most salient points are:
* The information that "the version must be non-negative" is true but
incomplete, so it isn't really helpful. At least I can't imagine a scenario
where an API user would benefit from knowing that "-1" is not a valid version
number but not knowing that "+6" is invalid.
* The "unsigned" modifier there is little better in conveying information to
the user than a comment, because C doesn't enforce it, silently converting
between signed and unsigned. (It's possible to get warnings from most
compilers. It's common to get a warning for passing a negative constant, but
warnings for passing possibly-negative variables are usually turned off because
there are too many instances where we need the silent conversion.)
* Mixing of unsigned and signed integers in C (e.g. acquiring a version number
from a signed source and then comparing it or converting it to unsigned for
this API) generally leads to more possibilities for programmer error than one
might expect, due mainly to the silent "promotion" of signed values to
unsigned. Sticking with plain "int" helps to avoid such problems.
> I actually don't care about changing it, i just want to know why you
> think it does more harm than good (particularly when signed and unsigned
> types have very different semantics in C)
OK. I hope the above is sufficient explanation.
>>>+ /* First thing in the string is the original length. */
>>>+ in->data = (char *)decode_size (&len, (unsigned char *)in->data,
>>>+ (unsigned char *)in->data+in->len);
>>>+ /* We need to subtract the size of the encoded original length off the
>>>+ * still remaining input length. */
>>>+ in->len -= (in->data - oldplace);
>>>+ if (in->len == len)
>>>+ {
>>>+ svn_stringbuf_appendstr (out, in);
>>
>>I wonder if we could easily avoid copying all the data here. It seems a shame
>>to be doing so. Maybe change this function's interface to output a pointer to
>>the "out" string, so that it can output the "in" string without copying it.
>
> IMHO, it's not worth it right now. I'd wait till it was a problem to
> muck up the code so it has to do it's own allocation in the common case,
> so that it can avoid copying in the *uncommon* case.
No such change of allocation is necessary. Let the output parameter be a
pointer to a pointer to a string which must have been allocated by the caller.
Then this function can just use that string if it needs to copy, or can just
change the pointer to point to the input string otherwise.
(I'm still not saying that _needs_ to be done.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Dec 15 16:41:51 2005