On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> On 02.12.2010 07:57, Blair Zajac wrote:
> > On 12/1/10 4:38 PM, stef...@apache.org wrote:
> >> Author: stefan2
> >> Date: Thu Dec 2 00:38:17 2010
> >> New Revision: 1041230
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1041230&view=rev
> >> Log:
> >> Fix the svn_checksum_to_cstring() docstring to actually say what
> >> was intended. Also, make clear that the behavior is new for 1.7 and
> >> trying to use it in 1.6 will cause segfaults.
> >>
> >> * subversion/include/svn_checksum.h
> >> (svn_checksum_to_cstring): fix docstring
> >
> > What happens if somebody makes a svn tool that is compiled and built
> > against the new 1.7 behavior and then it is backported to 1.6, it may
> > core dump.
> >
> > Should we add a svn_checksum_to_cstring2() instead with the new
> > behavior or backport this change to 1.6? But even then we'll have 1.6
> > versions with different behavior. It seems making a new
> > svn_checksum_to_cstring2() is better.
> I've felt kind of uneasy about that issue as well.
> However, it would have been difficult to implement
> a deprecated svn_checksum_to_cstring() in terms
> of svn_checksum_to_cstring2().
I don't think Blair was concerned about how the function is implemented,
but about the API promises. (He can correct me if I'm wrong.)
> For instance: [...]
> {
> if (checksum == NULL)
> abort();
>
> return svn_checksum_to_cstring2(checksum, pool);
> }
>
> does not have the exact same behavior as the old
> implementation. [...]
That doesn't matter. Passing checksum=NULL was not a supported usage in
1.6.x so there is no need to preserve compatibility with that failure
mode. It would be perfectly acceptable to implement it as:
const char *
svn_checksum_to_cstring(const svn_checksum_t *checksum,
apr_pool_t *pool)
{
return svn_checksum_to_cstring2(checksum, pool);
}
But, as I said in my previous reply in this thread, I think it's fine to
just change the existing API as you have done.
- Julian
