Re: [PATCH] String creation -- ensuring space for the NUL

Branko Čibej wrote:

> Julian Foad wrote:
>> We discovered some bugs recently [1,2] with use of svn_string.h functions,
>> where space for the terminating null character was sometimes not being
>> allocated.  [...]
>>
>>   * Introduce revved API svn_stringbuf_ensure2() that promises to make
>>     space for NUL.
>>
>>   * Make the old svn_stringbuf_ensure() provide space for NUL even though
>>     it doesn't promise to do so, to help remaining buggy callers to 'just
>>     work', since doing so is harmless and it was our inconsistent API that
>>     led to the misunderstanding.
>
> Please explain again, why do we need the revved _ensure2 at all? Can you
> think of any possible way that even marginally sane code would break if
> we just fixed the docs and behaviour of _ensure?

Valid question.  There's no way such a change would break code; the reason is entirely about being explicit so as to avoid confusion.  Quoting myself from an earlier email:

The point of defining the _ensure2() API is so we can cleanly change callers to not add 1, without them contradicting the old API.  (It would add confusion for people reading the code if we removed the caller's "+1" and didn't rev the name.)

For example, imagine your colleage Joe is maintaining svn a year or two from now, or some third-party Subversion client, and comes across a call to "svn_stringbuf_ensure(foo + 1 + bar)".  I don't want Joe to go through a thought process like this:

  "Yep, we're going to concatenate a 'foo' and a separator character and a 'bar', and the function promises to allocate its own space for the terminator.  Oh, no, wait-a-sec... this is the maintenance branch I'm looking at, which is still built against Subversion 1-point-errm-7, I think, which means that call might NOT be safe.  Ugh, WHEN was it that they changed that API's behaviour?"

[...]
> Just treat this as an API spec bug, fix the existing function and be
> done with it.

That would be fine, functionally.  However, I think it's a little better to spend the extra 5 minutes to rev the API for the clarity it brings.

> 5 minutes' work instead of never-ending code churn.

Never-ending?  Revving one little API that has about 20 calls in our entire code base is not really such a terrible burden of churn is it, if I'm volunteering to do the work (which I am)?

- Julian
Received on 2012-02-14 14:42:33 CET