Stefan Fuhrmann wrote on Tue, Aug 17, 2010 at 23:39:37 +0200: > Daniel Shahaf wrote: >> stef...@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.
