[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r985487 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/io.c libsvn_subr/stream.c

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Wed, 18 Aug 2010 17:50:18 +0300

Stefan Fuhrmann wrote on Tue, Aug 17, 2010 at 23:39:37 +0200:
> Daniel Shahaf wrote:
>> stefan2_at_apache.org wrote on Sat, Aug 14, 2010 at 13:20:22 -0000:
>>> URL: http://svn.apache.org/viewvc?rev=985487&view=rev
>>> +++ subversion/branches/performance/subversion/include/svn_io.h Sat Aug 14 13:20:22 2010
>>> +svn_io_file_read_full2(apr_file_t *file,
>>> + void *buf,
>>> + apr_size_t nbytes,
>>> + apr_size_t *bytes_read,
>>> + svn_boolean_t eof_is_ok,
>>> + apr_pool_t *pool);
>>
>> Why didn't you deprecate svn_io_file_read_full() in the same commit?
>>
> By the time I wrote that code almost 3 months ago, I tried to minimize
> the impact (code churn) on the code base.

When an API is revved, marking its old version deprecated and rewriting
it as a wrapper isn't considered churn.

Upgrading all existing calls is a bit more noise, but in general we
still do it (although not in tests). Usually I try to do that in
a separate commit.

>> Usually we do it as follows:
>>
>> + the declaration of svn_io_file_read_full2() is *above* that
>> of svn_io_file_read_full (not critical)
>> + mark the old function SVN_DEPRECATED (preprocessor symbol) and
>> doxygen @deprecated
>> + change the doc string of the old function to be a diff against the new
>> function's docs
>> + in the appropriate *.c file, upgrade the definition in-place
>> + re-define the old function in lib_$foo/deprecated.c as a wrapper around the
>> new function
>>
> Took me a number of commits but it should be o.k. by r986492.
>> Why you didn't make svn_io_file_read_full() a deprecated wrapper around
>> svn_io_file_read_full2()?
>>
> See above ;)
>

Thanks! And I hope you don't see this as procedural nitpicks --- I was
just highlighting the conventions we follow when revving an API.

> -- Stefan^2.

In other news: I'm a bit concerned about not seeing much
activity/discussions about merging some of your work to trunk. Perhaps
it'll be a good idea to start reviewing and merging some parts of it
now? (in parallel to your committing further new work to the branch)

For a change to be applied to trunk, you'll have to get a full
committer's +1 on that change.
Received on 2010-08-18 16:53:13 CEST

This is an archived mail posted to the Subversion Dev mailing list.