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