On Wed, Apr 18, 2012 at 09:47, Hyrum K Wright <hyrum.wright_at_wandisco.com> wrote:
> On Wed, Apr 18, 2012 at 8:18 AM, Julian Foad <julianfoad_at_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
Received on 2012-04-18 18:45:17 CEST