On Wed, Apr 18, 2012 at 09:47, Hyrum K Wright <hyrum.wri...@wandisco.com> wrote:
> On Wed, Apr 18, 2012 at 8:18 AM, Julian Foad <julianf...@btopenworld.com>
> wrote:
>...
>> In response to consistency: after writing the above I've just realized why
>> this function really is different from the others. Most of the checksum
>> functions that take a 'kind' input will create whatever kind you like,
>> independently of their other params. On the other hand, *this* function
>> takes two parameters that are required to agree with each other: the
>> 'digest' parameter has to point to an MD5 digest iff 'kind'=md5, and to a
>> SHA1 digest iff kind=sha1. It's not like 'const char *digest' is the
>> universal standard external representation of an arbitrary checksum kind;
>> although it would be possible for any checksum kind to have an external repr
>> that can be addressed as 'char *', the next kind we add might prefer to use
>> a different external representation such as 'struct foo'. So it's not wierd
>> to have one 'checksum' constructor function per checksum kind. That would
>> be the normal pattern for constructor functions.
>
> That argument makes sense.
Agreed.
I also dislike functions that have this pattern:
function(type, ...)
{
switch (type)
case 1:
do_something_for_1()
break
case 2:
do_something_for_2()
break;
}
IOW, just unroll the switch statement and make for_1 and for_2
publicly available. That is the API Julian suggested. And when TYPE is
a constant, it *really* makes sense to unroll.
One more thing: do we actually need svn_checksum__from_digest_sha1()
?? Are there any such digests anywhere in our system? We certainly
used MD5 digests in lots of our APIs and in our storage. But SHA1
digests?
Cheers,
-g
